Level: an async option on load/reload/next/previous, and a microtask deferral

[obiot]

Closes #1646.

The timer

level.load() deferred its work with a timer so the current frame could unwind before the world is reset. The deferral is still needed — it is routinely called from a trigger handler mid-loop, and safeLoadLevel resets and destroys the very container the loop may be iterating, while state.stop() only sets a flag.

The timer itself is a 2011 artefact: that line and its comment trace to v0.9.0, four years before promises existed. Browsers clamp a timer to ≥1 s in a background tab, so a load queued as the tab hides sat behind that clamp. A microtask drains when the JS stack empties — the end of the rAF callback holding update and draw — so it unwinds the frame identically and is not clamped.

The no-loop branch stays synchronous exactly as before.

The async option

load, reload, next and previous each keep one name and gain a flag:

level.load("map1");                          // boolean, exactly as before
await level.load("map1", { async: true });   // Promise<boolean>
if (await level.next({ async: true })) { … }

options.onLoaded still fires either way, so the forms mix.

No type break. The signatures are preserved as JSDoc @overload pairs, not a boolean | Promise<boolean> union — a union fails every existing const ok: boolean = level.load(id) with TS2322, which I verified before choosing. The overload form compiles both that and the awaited call against the real emitted build:

const ok: boolean = level.load("map1");                       // still compiles
const a: boolean = await level.load("map1", { async: true });  // Promise<boolean>

Running out of levels still reports false rather than rejecting — reaching the end of a game is an ordinary outcome. An unknown level id throws synchronously in both forms: that is a typo, not a load failure, and it should not need await to surface.

The bounds check next and previous each spelled out is now a shared levelIdAt(offset) helper, so the two cannot drift.

Trigger stops rewriting its caller's options

The fade/mask path sequenced hide → load → reveal by replacing settings.onLoaded with its own function and calling the user's from inside — mutating an option object the caller owns. Awaiting the load removes the interception.

The viewport is deliberately re-read after the load: game.reset() reassigns app.viewport, which is precisely why the callback this replaces read it late.

The cost of putting the switch in the options

await level.load(id) without the flag is silent, because await true is valid JavaScript. It happens to be harmless today — the deferral is a single microtask queued before the await's continuation, so the load still runs first — but that is incidental ordering, not a contract. Documented on the LevelLoadOptions typedef and pinned by a test that records exactly this.

Tests

No spec called level.load() at all before this, so both files are new — 24 tests across the legacy contract, the flag, the scheduling, the reload/next/previous paths, and the trigger.

Nine mutations of the changed behaviour fail as they should: the flag ignored, the microtask reverted to a timer, the unknown id rejecting instead of throwing, the loop not stopped, the synchronous branch throwing instead of rejecting, next/previous off-by-one, the onLoaded wrap reintroduced, and a stale viewport captured before the load.

A tenth (rewriting the bounds helper as levelIdx[index] ?? null) survives, and is an equivalent mutant rather than a gap: out-of-range array access is already undefined, so the explicit bounds check is belt-and-braces. The observable behaviour is covered by the two off-by-one mutations.

The viewport guard is a source check rather than a behavioural test — the reveal only runs when the hide tween completes, which needs a live game loop the suite does not have. It was vacuous on the first attempt (the explanatory comment in the inspected slice contained the string it asserted on), so comment lines are stripped before matching, and it was re-verified against the mutation.

Also

reload() was documented as returning object — "the current level" — but returns whatever load() returns. The 2011 original returned nothing at all, so the declaration was never right. Corrected to boolean; getCurrentLevel() is the call that hands back the level object.

Verification

276 files, 6716 tests, 0 failures. Lint 0 errors, build clean, emitted overloads and legacy compatibility both checked against the built types.

[Copilot]

🟡 Changes recommended

There are a few correctness/docs issues to address (notably loadAsync() error-surface consistency and a non-Markdown {@link ...} tag in the changelog).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR modernizes level-loading scheduling by replacing the legacy setTimeout deferral with a microtask-based deferral, and introduces an awaitable level.loadAsync() to let callers observe completion without changing the existing level.load() boolean-return contract. It also updates Trigger’s level-transition path to avoid mutating caller-owned settings.onLoaded, and adds new Vitest coverage for the behavior and scheduling.

Changes:

• Add level.loadAsync(levelId, options): Promise<void> while keeping level.load()’s boolean return and “fire-and-forget” behavior.
• Replace timer-based deferral with a microtask deferral when the game loop is running; preserve synchronous load behavior when no loop is running.
• Refactor trigger level transitions to await the load (via loadAsync) instead of wrapping/overwriting settings.onLoaded, and add new tests.

File summaries

File	Description
packages/melonjs/src/level/level.js	Adds loadAsync(), refactors load() to delegate and rethrow failures, and replaces timer deferral with a microtask deferral when the loop is running.
packages/melonjs/src/renderable/trigger.js	Updates transition path to use level.loadAsync(...).then(...) for reveal instead of rewriting settings.onLoaded.
packages/melonjs/tests/level_load_async.spec.js	New tests for loadAsync, legacy load() contract, and microtask-vs-timer scheduling behavior.
packages/melonjs/tests/trigger_level_change.spec.js	New tests ensuring trigger transition behavior doesn’t overwrite caller callbacks and that load is deferred until hide completes.
packages/melonjs/CHANGELOG.md	Documents the new API and the background-tab timer clamp fix.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[obiot]

Added the three missing twins — reloadAsync(), nextAsync(), previousAsync(). loadAsync() alone left a game able to await its first level but not a reload or a level transition.

Each resolves with exactly what its synchronous counterpart returns, so a port is mechanical:

if (level.next())              →   if (await level.nextAsync())

That is why loadAsync() now resolves true rather than void — the rule is worth more than the marginally noisier type.

Running out of levels resolves false without loading anything rather than rejecting. next() returns false there, and reaching the end of a game is an ordinary outcome, not an error.

Emitted types, verified from build/level/level.d.ts — all four originals unchanged:

load	boolean	loadAsync → Promise<boolean>
reload	object	reloadAsync → Promise<boolean>
next	boolean	nextAsync → Promise<boolean>
previous	boolean	previousAsync → Promise<boolean>

Six more tests, and five mutations of the new behaviour all fail as they should: nextAsync resolving true at the end, loading past the end, a previousAsync off-by-one at the start, reloadAsync not reloading, and loadAsync resolving undefined.

Noticed but deliberately not fixed: reload() declares object above, from a stale @returns {object} the current level — it actually returns whatever load() returns. Correcting it would change an emitted type, which is precisely what this PR is careful not to do, so it is left for a separate call.

Suite now 276 files, 6713 tests, 0 failures.

[Copilot]

🟡 Changes recommended

It introduces unconditional queueMicrotask usage (risking runtime ReferenceError in unsupported environments) and the PR description’s stated scope conflicts with the included async twin APIs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

packages/melonjs/src/level/level.js:198

• load() rethrows rejections using queueMicrotask, but this global is not guaranteed to exist in all runtimes. If it’s missing, a rejected loadAsync() will cause a ReferenceError here instead of surfacing the original failure. Consider a small fallback to setTimeout when queueMicrotask is unavailable.

	load(levelId, options) {
		// Fire-and-forget by contract: this returns `true`, not the promise, so
		// existing (including typed) callers are unaffected. Use `loadAsync()`
		// to await the load. The rejection is rethrown on a clean stack so a
		// failure still surfaces as an uncaught error the way it did when the
		// deferral was a timer, rather than as a silent unhandled rejection.
		this.loadAsync(levelId, options).catch((error) => {
			queueMicrotask(() => {
				throw error;
			});
		});

packages/melonjs/src/renderable/trigger.js:209

• This catch path rethrows via queueMicrotask, which may be undefined in some runtimes; in that case the code would throw a ReferenceError and potentially mask the real level-load failure. Using a simple fallback (e.g. setTimeout) keeps the intended “uncaught” loudness without requiring queueMicrotask support.

							})
							.catch((error) => {
								// same loudness as the fire-and-forget form
								queueMicrotask(() => {
									throw error;
								});
							});

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core level-loading scheduling and trigger transition sequencing in a way that can have subtle runtime/event-loop effects best validated by a human reviewer.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

level.load() now forces some previously synchronous failures (when the loop isn’t running) to become asynchronous throws via loadAsync().catch(...), which is a behavior/contract change that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[obiot]

Responses to the review, plus a design change since it was written.

queueMicrotask → Promise.resolve().then(() => { throw error }) (two comments): declining. The two are not equivalent for this purpose, measured in the browser:

queueMicrotask(() => { throw e })          → ["uncaught-error"]
Promise.resolve().then(() => { throw e })  → ["unhandled-rejection"]

That rethrow exists specifically to preserve the uncaught error the timer-based version produced. The suggested form would turn it into an unhandled rejection, which is the behaviour change the code is there to avoid. On availability: queueMicrotask is baseline in every browser since 2018 and Node 11, comfortably older than the ES2022 target this package already requires.

loadAsync() error-surface inconsistency: valid, fixed. The deferred branch produced a rejection while the synchronous one let the exception escape the call, so the surface depended on state.isRunning() — and .catch() could never see the synchronous case, since the throw beat the handler being attached. The synchronous branch now returns a rejected promise, with a test asserting both branches reject identically. Good catch.

{@link ...} in the changelog: already gone — the entry had been rewritten before the review ran.

PR description out of step with scope: it was, and it is now further out of step, because the API changed after this review. The description has been rewritten.

Design change: the four *Async siblings are gone. Eight loading methods for four operations was too much surface, so the switch moved into the options object: level.load(id, { async: true }), and the same on reload/next/previous.

The signatures are preserved as JSDoc @overload pairs rather than a boolean | Promise<boolean> union — the union form fails every existing const ok: boolean = level.load(id) with TS2322, verified with tsc before choosing. The overload form compiles both the legacy call and the awaited one against the real emitted build.

[Copilot]

🟡 Changes recommended

There are a few correctness/robustness issues in the newly added docs/tests and a missing queueMicrotask fallback that could cause runtime failures in unsupported environments.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

packages/melonjs/src/renderable/trigger.js:208

• queueMicrotask is used here to rethrow load failures, but it isn’t feature-detected or polyfilled in this repo. In runtimes without queueMicrotask, this catch handler will throw a ReferenceError and may mask the original error. Consider falling back to setTimeout for the rethrow path.

								// same loudness as the fire-and-forget form
								queueMicrotask(() => {
									throw error;
								});

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are cohesive, preserve legacy behavior, and are backed by targeted tests covering the new async contract, scheduling, and Trigger sequencing.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are a couple of correctness/maintainability issues in newly added documentation/tests (e.g., an inaccurate await note and a brittle source-string assertion) that should be adjusted before merge.

Review details

Suppressed comments (2)

packages/melonjs/src/level/level.js:111

• The LevelLoadOptions note says that await level.load(id) "resolves immediately — so the level will not be loaded yet", but that’s not generally true (it loads synchronously when the loop is stopped, and while running it may complete before the await continuation due to microtask ordering). The docs should instead say that omitting async: true does not await completion, and callers must not rely on ordering.

 * Note that awaiting a call WITHOUT `async: true` is not an error — `await true`
 * is valid and resolves immediately — so the level will not be loaded yet. Pass
 * the flag whenever you intend to await.

packages/melonjs/tests/trigger_level_change.spec.js:104

• This source-based assertion searches for an exact, whitespace-sensitive substring (including { ...settings, async: true } formatting). That makes the test fragile to harmless formatting/refactor changes in trigger.js. Consider using a regex/search() that tolerates whitespace/newlines while still asserting .load(gotolevel, … async: true …) ordering relative to the reveal.

		const load = triggerSource.indexOf(
			"load(gotolevel, { ...settings, async: true })",
		);
		const reveal = triggerSource.indexOf("addCameraEffect", load);
		expect(load).toBeGreaterThan(-1);

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new queueMicrotask-based rethrow paths should defensively handle runtimes where queueMicrotask is unavailable to avoid masking failures with a ReferenceError/unhandled rejection.

Review details

Suppressed comments (2)

packages/melonjs/src/level/level.js:270

• The fire-and-forget path rethrows failures via queueMicrotask, but the project explicitly avoids bundling polyfills. If queueMicrotask is missing at runtime, this handler can throw a ReferenceError and potentially hide the original error (or produce an unhandled rejection). Consider falling back to setTimeout when queueMicrotask is unavailable.

				queueMicrotask(() => {
					throw error;

packages/melonjs/src/renderable/trigger.js:207

• queueMicrotask is used to rethrow load failures, but it is a relatively new platform API and the engine intentionally avoids shipping polyfills. If queueMicrotask is unavailable in a runtime that can still parse the bundle, this catch handler will throw a ReferenceError and can turn the original failure into an unhandled rejection (or mask it). Consider a small fallback to setTimeout when queueMicrotask is missing.

								queueMicrotask(() => {
									throw error;

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[obiot]

Second round.

Options doc claiming the level is unloaded when you await without the flag — correct, fixed. That is the same error I had already corrected in the skills and missed in this copy: with no loop the load is synchronous, and with one running the deferral is a single microtask queued ahead of the await's continuation, so it finishes first either way. The point still stands — without the flag there is no completion point to await — but the ordering is incidental, and the doc now says that rather than the opposite.

Brittle source-substring test — fair, fixed. Anchored on load(gotolevel instead of the full argument text, so reformatting or an added option cannot fail it. Re-verified that it still fails when the viewport is captured before the load, which is the only thing it exists for.

load() delegating to loadAsync() losing the synchronous throw — resolved, though by redesign rather than by patch. loadAsync() no longer exists: the switch moved into the options object, and the no-loop non-async path is now literally

safeLoadLevel(levelId, options);
return true;

so a setup-time failure still throws synchronously and try { level.load(id) } catch still works. The promise form wraps the same call and rejects instead, which keeps the two consistent — that was the earlier review comment, and both are now satisfied at once.

queueMicrotask fallback — still declining, same reasoning as before. It is baseline in Chrome 71, Firefox 69, Safari 12.1 and Node 11, all from 2018–2019; ES2022 syntax this package already emits (class fields, private members) needs strictly newer engines than that, so there is no runtime that can parse the bundle but lacks the function. Adding a setTimeout fallback would also reintroduce the background-tab clamp on the error path this PR exists to remove. Worth restating that the choice is not stylistic: measured in-browser, queueMicrotask(() => { throw e }) produces an uncaught error while Promise.resolve().then(() => { throw e }) produces an unhandled rejection, and preserving the former is the point.

[Copilot]

🔵 Needs a closer look

New queueMicrotask usage is unguarded and not polyfilled in this repo, which can cause runtime TypeErrors that mask the original load error in environments lacking queueMicrotask.

Review details

Suppressed comments (2)

packages/melonjs/src/level/level.js:276

• queueMicrotask is used to rethrow async load failures, but it’s not polyfilled anywhere in this repo. In runtimes where queueMicrotask is undefined, this will throw a TypeError and can mask the original load failure. Consider a small fallback so errors remain loud without introducing a new hard runtime requirement.

			deferred.catch((error) => {
				queueMicrotask(() => {
					throw error;
				});
			});

packages/melonjs/src/renderable/trigger.js:209

• This .catch rethrows via queueMicrotask, but queueMicrotask is not polyfilled in melonJS (polyfills are intentionally Canvas/DOM-only). If a consumer runs in an environment without queueMicrotask, this handler itself throws and may hide the original level-load error. Add a small fallback (e.g., to setTimeout) to keep the error reporting robust.

							.catch((error) => {
								// same loudness as the fire-and-forget form
								queueMicrotask(() => {
									throw error;
								});
							});

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are internally consistent with the stated compatibility goals, and the new/updated behavior is thoroughly covered by targeted tests for scheduling, ordering, and trigger integration.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite