From f913d6363e8a019042dc246d51c541026ce63a8a Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Sun, 2 Aug 2026 09:01:38 +0200 Subject: [PATCH 1/6] Harden the task queue against synchronously-throwing tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A task that threw synchronously used to (a) abort the rest of its batch and (b) leave the queue's scheduling flag stuck `true` forever, which permanently froze every queued lifecycle operation on the page. Its `add()` promise also stayed pending forever, so awaiters hung. - `Queue`/`SmartQueue`: run each task in `try/catch` so one throw can no longer abort the batch; reset the scheduling flag in `finally` and re-arm the flush. A throwing custom scheduler no longer wedges either. - `add()` now rejects on a synchronous throw — consistent with an async task that returns a rejected promise — instead of hanging or silently resolving, so a failure is never mistaken for success. - Handle the rejection at the call sites rather than changing behaviour by hiding it: the fire-and-forget queue calls (`mutationCallback`, the async-child hooks) route errors to a warn helper, and the lifecycle boundaries (`$mount`/`$update`/`$destroy`/`$terminate`) surface the error and no longer falsely mark a component as mounted. Adds regression tests for batch isolation, scheduler-throw recovery, reject consistency, exactly-once error reporting, and the not-falsely-mounted lifecycle behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep --- packages/js-toolkit/Base/Base.ts | 119 ++++++++++++------ .../Base/managers/ChildrenManager.ts | 14 ++- packages/js-toolkit/Base/utils.ts | 23 +++- packages/js-toolkit/utils/Queue.ts | 56 +++++++-- packages/js-toolkit/utils/SmartQueue.ts | 25 +++- packages/tests/Base/Base.spec.ts | 72 +++++++++++ packages/tests/utils/Queue.spec.ts | 107 ++++++++++++++++ packages/tests/utils/SmartQueue.spec.ts | 68 ++++++++++ 8 files changed, 424 insertions(+), 60 deletions(-) diff --git a/packages/js-toolkit/Base/Base.ts b/packages/js-toolkit/Base/Base.ts index 533b56133..7ff66eb4d 100644 --- a/packages/js-toolkit/Base/Base.ts +++ b/packages/js-toolkit/Base/Base.ts @@ -7,6 +7,7 @@ import { deleteInstance, addToRegistry, hasInstance, + reportQueuedTaskError, } from './utils.js'; import { ChildrenManager, @@ -474,18 +475,31 @@ export class Base { this.__debug('$mount'); } - await Promise.all([ - addToQueue(() => this.__children.registerAll()), - addToQueue(() => this.__refs.registerAll()), - addToQueue(() => this.__events.bindRootElement()), - addToQueue(() => this.__services.enableAll()), - addToQueue(() => this.__children.mountAll()), - ]); - - await addToQueue(() => { - this.$isMounted = true; - this.__callMethod('mounted'); - }); + try { + await Promise.all([ + addToQueue(() => this.__children.registerAll()), + addToQueue(() => this.__refs.registerAll()), + addToQueue(() => this.__events.bindRootElement()), + addToQueue(() => this.__services.enableAll()), + addToQueue(() => this.__children.mountAll()), + ]); + + await addToQueue(() => { + this.$isMounted = true; + this.__callMethod('mounted'); + }); + } catch (error) { + // A queued mount task failed. Leave the component in an honest state + // instead of emitting `after-mounted` as if init completed: `$isMounted` + // keeps whatever value it reached (still `false` when the failure happened + // during wiring), so the component is not falsely reported as mounted. + // The error is surfaced through the log rather than re-thrown — the queued + // task ran detached, and most callers ($mount is often fire-and-forget) + // would only turn a rejection into an unhandled rejection. + this.__isMounting = false; + reportQueuedTaskError(error); + return this; + } this.__isMounting = false; this.$emit('after-mounted'); @@ -502,19 +516,26 @@ export class Base { this.__debug('$update'); } - await Promise.all([ - // Undo - addToQueue(() => this.__refs.unregisterAll()), - addToQueue(() => this.__services.disableAll()), - // Redo - addToQueue(() => this.__children.registerAll()), - addToQueue(() => this.__refs.registerAll()), - addToQueue(() => this.__services.enableAll()), - // Update - addToQueue(() => this.__children.updateAll()), - ]); - - await addToQueue(() => this.__callMethod('updated')); + try { + await Promise.all([ + // Undo + addToQueue(() => this.__refs.unregisterAll()), + addToQueue(() => this.__services.disableAll()), + // Redo + addToQueue(() => this.__children.registerAll()), + addToQueue(() => this.__refs.registerAll()), + addToQueue(() => this.__services.enableAll()), + // Update + addToQueue(() => this.__children.updateAll()), + ]); + + await addToQueue(() => this.__callMethod('updated')); + } catch (error) { + // Surface a failed queued update task instead of rejecting: `$update` is + // commonly called fire-and-forget, so a rejection would only produce an + // unhandled rejection. + reportQueuedTaskError(error); + } return this; } @@ -535,14 +556,23 @@ export class Base { this.$emit('before-destroyed'); this.$isMounted = false; - await Promise.all([ - addToQueue(() => this.__events.unbindRootElement()), - addToQueue(() => this.__refs.unregisterAll()), - addToQueue(() => this.__services.disableAll()), - addToQueue(() => this.__children.destroyAll()), - ]); - - await addToQueue(() => this.__callMethod('destroyed')); + try { + await Promise.all([ + addToQueue(() => this.__events.unbindRootElement()), + addToQueue(() => this.__refs.unregisterAll()), + addToQueue(() => this.__services.disableAll()), + addToQueue(() => this.__children.destroyAll()), + ]); + + await addToQueue(() => this.__callMethod('destroyed')); + } catch (error) { + // A queued teardown task failed. Do not emit `after-destroyed` (which + // removes the instance from the global storage) as if teardown completed; + // surface the error instead of re-throwing so fire-and-forget callers do + // not produce an unhandled rejection. + reportQueuedTaskError(error); + return this; + } this.$emit('after-destroyed'); @@ -558,14 +588,21 @@ export class Base { this.__debug('$terminate'); } - await Promise.all([ - // First, destroy the component. - addToQueue(() => this.$destroy()), - // Execute the `terminated` hook if it exists - addToQueue(() => this.__callMethod('terminated')), - // Delete instance - addToQueue(() => this.$el.__base__.set(this.$config.name, 'terminated')), - ]); + try { + await Promise.all([ + // First, destroy the component. + addToQueue(() => this.$destroy()), + // Execute the `terminated` hook if it exists + addToQueue(() => this.__callMethod('terminated')), + // Delete instance + addToQueue(() => this.$el.__base__.set(this.$config.name, 'terminated')), + ]); + } catch (error) { + // Surface a failed queued termination task instead of rejecting: + // `$terminate` is called fire-and-forget (e.g. from `mutationCallback`), + // so a rejection would only produce an unhandled rejection. + reportQueuedTaskError(error); + } } /** diff --git a/packages/js-toolkit/Base/managers/ChildrenManager.ts b/packages/js-toolkit/Base/managers/ChildrenManager.ts index 47d250410..6a3b836f2 100644 --- a/packages/js-toolkit/Base/managers/ChildrenManager.ts +++ b/packages/js-toolkit/Base/managers/ChildrenManager.ts @@ -1,6 +1,6 @@ import type { Base, BaseConstructor, BaseAsyncConstructor, BaseEl } from '../index.js'; import { AbstractManager } from './AbstractManager.js'; -import { getComponentElements, addToQueue } from '../utils.js'; +import { getComponentElements, addToQueue, reportQueuedTaskError } from '../utils.js'; /** * Children manager. @@ -204,9 +204,15 @@ export class ChildrenManager extends AbstractManager { for (const name of this.registeredNames) { for (const instance of this.props[name]) { if (instance instanceof Promise) { - instance.then((resolvedInstance) => - addToQueue(() => this.__triggerHook(hook, resolvedInstance, name)), - ); + // Fire-and-forget: this branch does not push into `promises`, so its + // result is never awaited. Attach a `.catch()` so a failure in the + // async child resolution or its queued hook is logged instead of + // becoming an unhandled rejection. + instance + .then((resolvedInstance) => + addToQueue(() => this.__triggerHook(hook, resolvedInstance, name)), + ) + .catch(reportQueuedTaskError); } else { promises.push(addToQueue(() => this.__triggerHook(hook, instance, name))); } diff --git a/packages/js-toolkit/Base/utils.ts b/packages/js-toolkit/Base/utils.ts index 642647568..83b9ef98d 100644 --- a/packages/js-toolkit/Base/utils.ts +++ b/packages/js-toolkit/Base/utils.ts @@ -22,6 +22,21 @@ export function addToQueue(fn: () => unknown) { return queue.add(fn); } +/** + * Report an error raised by a queued task. + * + * Queued work runs detached from its original call site — fire-and-forget + * mounts, mutation-driven auto-mounting, async children — so the returned + * promise is often discarded. When such a task fails, its promise rejects with + * no awaiter to observe it; routing it here keeps the failure visible instead + * of turning it into an unhandled rejection or, worse, silently swallowing an + * initialization error. It is reported unconditionally (not gated behind the + * `log` option) so a genuine failure is never lost in production. + */ +export function reportQueuedTaskError(error: unknown): void { + console.warn('[@studiometa/js-toolkit] A queued task failed:', error); +} + const selectors = new Map(); // Separator used for multi-component declaration in `data-component` attributeS. @@ -181,6 +196,10 @@ function registry() { } function mutationCallback() { + // Fire-and-forget: the returned promise is discarded, so route a rejection + // (a synchronously-throwing task) to the log instead of letting it become an + // unhandled rejection. `addToQueue` returns `undefined` in blocking mode, + // hence the optional chaining. addToQueue(() => { for (const [nameOrSelector, ctor] of registry()) { for (const el of getComponentElements(nameOrSelector)) { @@ -189,7 +208,7 @@ function mutationCallback() { } } } - }); + })?.catch(reportQueuedTaskError); addToQueue(() => { for (const instance of getInstances()) { @@ -197,7 +216,7 @@ function mutationCallback() { instance.$terminate(); } } - }); + })?.catch(reportQueuedTaskError); } export function addToRegistry(nameOrSelector: string, ctor: BaseConstructor) { diff --git a/packages/js-toolkit/utils/Queue.ts b/packages/js-toolkit/utils/Queue.ts index 7894c4a61..6fee9c252 100644 --- a/packages/js-toolkit/utils/Queue.ts +++ b/packages/js-toolkit/utils/Queue.ts @@ -40,8 +40,23 @@ export class Queue { * Add a task to the queue. */ add(task: () => unknown) { - const p = new Promise((resolve) => { - this.tasks.push(() => resolve(task())); + const p = new Promise((resolve, reject) => { + this.tasks.push(() => { + try { + resolve(task()); + } catch (err) { + // Reject the returned promise so a synchronously-throwing task fails + // exactly like an asynchronous one whose promise rejects — a caller + // doing rollback in `catch` behaves the same regardless of whether the + // task was declared `async`. The rejection is the single surfacing + // channel for the error: do not also re-throw here, otherwise `run()` + // would report the same error a second time via its `queueMicrotask`. + // Fire-and-forget callers attach their own `.catch()` (see + // `Base/utils.ts` and `ChildrenManager.ts`) so this never becomes an + // unhandled rejection. + reject(err); + } + }); }); this.scheduleFlush(); return p; @@ -56,18 +71,33 @@ export class Queue { } this.isScheduled = true; - this.waiter(() => this.flush()); + + try { + this.waiter(() => this.flush()); + } catch (err) { + // If the scheduler itself throws, reset the flag before re-throwing so a + // faulty `waiter` can not leave the queue permanently wedged (the flag + // stuck `true` would make every later `scheduleFlush()` early-return and + // never re-arm a flush). + this.isScheduled = false; + throw err; + } } /** * Flush current batch. */ flush() { - this.run(this.tasks.splice(0, this.concurrency)); - this.isScheduled = false; + try { + this.run(this.tasks.splice(0, this.concurrency)); + } finally { + // Always reset the scheduling flag and re-arm the next flush, even when a + // task throws, so a single faulty task can not permanently wedge the queue. + this.isScheduled = false; - if (this.tasks.length > 0) { - this.scheduleFlush(); + if (this.tasks.length > 0) { + this.scheduleFlush(); + } } } @@ -78,7 +108,17 @@ export class Queue { let task; // eslint-disable-next-line no-cond-assign while ((task = tasks.shift())) { - task(); + try { + task(); + } catch (err) { + // Defence in depth: tasks pushed by `add()` already isolate their own + // errors (they reject the returned promise and never throw here), so + // this only catches an unexpected throw. Keep draining the batch and + // surface the error asynchronously instead of aborting the whole flush. + queueMicrotask(() => { + throw err; + }); + } } } } diff --git a/packages/js-toolkit/utils/SmartQueue.ts b/packages/js-toolkit/utils/SmartQueue.ts index 1eff6e009..6ac0dccb4 100644 --- a/packages/js-toolkit/utils/SmartQueue.ts +++ b/packages/js-toolkit/utils/SmartQueue.ts @@ -32,11 +32,16 @@ export class SmartQueue extends Queue { * Flush current batch. */ flush() { - this.run(this.tasks); - this.isScheduled = false; + try { + this.run(this.tasks); + } finally { + // Always reset the scheduling flag and re-arm the next flush, even when a + // task throws, so a single faulty task can not permanently wedge the queue. + this.isScheduled = false; - if (this.tasks.length > 0) { - this.scheduleFlush(); + if (this.tasks.length > 0) { + this.scheduleFlush(); + } } } @@ -49,7 +54,17 @@ export class SmartQueue extends Queue { let now = start; // eslint-disable-next-line no-cond-assign while (now - start < LONG_TASK_DURATION && (task = tasks.shift())) { - task(); + try { + task(); + } catch (err) { + // Defence in depth: tasks pushed by `add()` already isolate their own + // errors (they reject the returned promise and never throw here), so + // this only catches an unexpected throw. Keep draining the batch and + // surface the error asynchronously instead of aborting the whole flush. + queueMicrotask(() => { + throw err; + }); + } now = performance.now(); } } diff --git a/packages/tests/Base/Base.spec.ts b/packages/tests/Base/Base.spec.ts index 2e0ca7d51..4d5cdd757 100644 --- a/packages/tests/Base/Base.spec.ts +++ b/packages/tests/Base/Base.spec.ts @@ -8,6 +8,7 @@ import { withExtraConfig, withName, } from '@studiometa/js-toolkit'; +import { nextTick } from '@studiometa/js-toolkit/utils'; import { h, mount } from '#test-utils'; let mockedIsDev = true; @@ -737,3 +738,74 @@ describe('A Base instance config', () => { process.env.NODE_ENV = 'test'; }); }); + +describe('The Base lifecycle error boundaries', () => { + it('should not falsely mark the component mounted when a queued mount task fails', async () => { + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + } + + const foo = new Foo(h('div')); + const error = new Error('enable boom'); + // Make a queued wiring task throw synchronously, mimicking e.g. + // `__services.enableAll()` failing during `$mount`. + vi.spyOn(foo.__services, 'enableAll').mockImplementation(() => { + throw error; + }); + + const afterMounted = vi.fn(); + foo.$on('after-mounted', afterMounted); + const warn = vi.spyOn(window.console, 'warn').mockImplementation(() => {}); + + // `$mount` resolves (never rejects) so a fire-and-forget mount can not turn + // into an unhandled rejection. + await expect(foo.$mount()).resolves.toBe(foo); + + // The component is left honestly unmounted... + expect(foo.$isMounted).toBe(false); + // ...`after-mounted` is not emitted as if init had completed... + expect(afterMounted).not.toHaveBeenCalled(); + // ...and the failure is surfaced through the log, not swallowed. + expect(warn.mock.calls.some((args) => args.includes(error))).toBe(true); + + warn.mockRestore(); + }); + + it('should not produce an unhandled rejection when a fire-and-forget mount fails', async () => { + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + + mounted() { + throw new Error('mounted boom'); + } + } + + const warn = vi.spyOn(window.console, 'warn').mockImplementation(() => {}); + const rejections: unknown[] = []; + const onRejection = (reason: unknown) => rejections.push(reason); + process.on('unhandledRejection', onRejection); + + // Fire-and-forget: the returned promise is discarded, like `mutationCallback`. + new Foo(h('div')).$mount(); + + // Let the queued mount tasks flush across several ticks and give any + // rejection a chance to surface. + for (let i = 0; i < 5; i += 1) { + // eslint-disable-next-line no-await-in-loop + await nextTick(); + } + await new Promise((resolve) => setTimeout(resolve, 0)); + + process.off('unhandledRejection', onRejection); + + expect(rejections).toHaveLength(0); + // The failure is still surfaced through the log. + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); +}); diff --git a/packages/tests/utils/Queue.spec.ts b/packages/tests/utils/Queue.spec.ts index 350231fa2..5aaac8de1 100644 --- a/packages/tests/utils/Queue.spec.ts +++ b/packages/tests/utils/Queue.spec.ts @@ -37,4 +37,111 @@ describe('The `Queue` class', () => { await p; expect(spy).toHaveBeenCalledTimes(1); }); + + it('should keep running the rest of the batch when a task throws', async () => { + // Use a manual waiter so all three tasks land in a single pending batch and + // one flush drains them together — this is what exercises the same-batch + // isolation (an immediate waiter would flush each `add()` on its own). + let scheduled: (() => void) | undefined; + const queue = new Queue(10, (cb) => { + scheduled = cb as () => void; + }); + const before = vi.fn(); + const after = vi.fn(); + const error = new Error('boom'); + + queue.add(before); + // The throwing task's promise rejects; observe it so it is not reported as + // an unhandled rejection. + const throwing = queue.add(() => { + throw error; + }); + throwing.catch(() => {}); + queue.add(after); + + // Nothing ran yet: all three tasks share the same pending batch. + expect(before).not.toHaveBeenCalled(); + + // Trigger the single flush that drains the whole batch. + scheduled?.(); + + // The throwing task must not abort the tasks queued before or after it. + expect(before).toHaveBeenCalledTimes(1); + expect(after).toHaveBeenCalledTimes(1); + + // The failure is surfaced through the task's own promise, which rejects. + await expect(throwing).rejects.toBe(error); + }); + + it('should not wedge the queue for future flushes when a task throws', () => { + const queue = new Queue(10); + + const throwing = queue.add(() => { + throw new Error('boom'); + }); + // Observe the rejection so it does not surface as an unhandled rejection. + throwing.catch(() => {}); + + // The scheduling flag must have been reset by the previous flush. + expect(queue.isScheduled).toBe(false); + + // A subsequent enqueue must still schedule and run. + const spy = vi.fn(); + queue.add(spy); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('should reject the promise of a synchronously-throwing task', async () => { + // The re-throw path in `run()` must not fire — the rejection is the single + // surfacing channel, so there is no double report. + const microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => {}); + const queue = new Queue(10); + const error = new Error('boom'); + + const p = queue.add(() => { + throw error; + }); + + // A synchronous throw rejects, exactly like an async task returning a + // rejected promise — the completion contract is consistent. + await expect(p).rejects.toBe(error); + + // The error is surfaced exactly once (through the rejection), not also via + // the global re-throw. + expect(microtask).not.toHaveBeenCalled(); + + microtask.mockRestore(); + }); + + it('should reject consistently for sync and async throws', async () => { + const queue = new Queue(10); + const error = new Error('boom'); + + const sync = queue.add(() => { + throw error; + }); + const async = queue.add(async () => { + throw error; + }); + + await expect(sync).rejects.toBe(error); + await expect(async).rejects.toBe(error); + }); + + it('should reset the scheduling flag when the scheduler throws', () => { + const error = new Error('scheduler boom'); + const queue = new Queue(1, () => { + throw error; + }); + + // The throw propagates to the caller... + expect(() => + queue.add(() => { + /* noop */ + }), + ).toThrow(error); + + // ...but the flag is reset so the queue is not permanently wedged. + expect(queue.isScheduled).toBe(false); + }); }); diff --git a/packages/tests/utils/SmartQueue.spec.ts b/packages/tests/utils/SmartQueue.spec.ts index 46571f549..be39e8370 100644 --- a/packages/tests/utils/SmartQueue.spec.ts +++ b/packages/tests/utils/SmartQueue.spec.ts @@ -25,4 +25,72 @@ describe('The `SmartQueue` class', () => { await nextTick(); expect(spy).toHaveBeenCalledTimes(4); }); + + it('should keep running the batch when a task throws', async () => { + const queue = new SmartQueue(); + const before = vi.fn(); + const after = vi.fn(); + const error = new Error('boom'); + + queue.add(before); + // The throwing task's promise rejects; observe it so it is not reported as + // an unhandled rejection. + const throwing = queue.add(() => { + throw error; + }); + throwing.catch(() => {}); + queue.add(after); + + await nextTick(); + + // Both the task before and after the throwing one must have run. + expect(before).toHaveBeenCalledTimes(1); + expect(after).toHaveBeenCalledTimes(1); + + // The failure is surfaced through the task's own promise, which rejects. + await expect(throwing).rejects.toBe(error); + }); + + it('should not wedge the queue for future flushes when a task throws', async () => { + const queue = new SmartQueue(); + const spy = vi.fn(); + + const throwing = queue.add(() => { + throw new Error('boom'); + }); + // Observe the rejection so it does not surface as an unhandled rejection. + throwing.catch(() => {}); + await nextTick(); + + // The scheduling flag must have been reset by the previous flush. + expect(queue.isScheduled).toBe(false); + + // A subsequent enqueue must still schedule and run. + queue.add(spy); + await nextTick(); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('should reject the promise of a synchronously-throwing task', async () => { + // The re-throw path in `run()` must not fire — the rejection is the single + // surfacing channel, so there is no double report. + const microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => {}); + const queue = new SmartQueue(); + const error = new Error('boom'); + + const p = queue.add(() => { + throw error; + }); + await nextTick(); + + // A synchronous throw rejects, exactly like an async task returning a + // rejected promise — the completion contract is consistent. + await expect(p).rejects.toBe(error); + + // The error is surfaced exactly once (through the rejection), not also via + // the global re-throw. + expect(microtask).not.toHaveBeenCalled(); + + microtask.mockRestore(); + }); }); From 96be5e62097f45e7305b4cc792f426b31cbe6047 Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Fri, 7 Aug 2026 10:35:38 +0200 Subject: [PATCH 2/6] Trim the queue fix to the load-bearing change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Queue.ts` is the only writer to `this.tasks` package-wide, and after the `add()` fix the closure it pushes provably cannot throw. The extra guards added around it were therefore unreachable through the public API: - `Queue.flush()`'s try/finally - `Queue.run()`'s try/catch and `queueMicrotask` re-throw - both `SmartQueue` overrides, which now simply inherit the fix `scheduleFlush()`'s `isScheduled` reset stays: `waiter` is a public constructor parameter, so a throwing scheduler is reachable. The task it already pushed stays queued on purpose — it runs on the next flush that does get scheduled, and its promise never reached the caller, so rejecting it could only produce an unobservable rejection. Also fix `SmartQueue.spec.ts` leaving a rejection unobserved, which made a root-level `vitest run packages/tests/utils/SmartQueue.spec.ts` exit 1. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3 --- packages/js-toolkit/utils/Queue.ts | 50 +++++++++---------------- packages/js-toolkit/utils/SmartQueue.ts | 25 +++---------- packages/tests/utils/Queue.spec.ts | 47 ++++++++++++----------- packages/tests/utils/SmartQueue.spec.ts | 28 +++++++------- 4 files changed, 61 insertions(+), 89 deletions(-) diff --git a/packages/js-toolkit/utils/Queue.ts b/packages/js-toolkit/utils/Queue.ts index 6fee9c252..3a096aadf 100644 --- a/packages/js-toolkit/utils/Queue.ts +++ b/packages/js-toolkit/utils/Queue.ts @@ -45,15 +45,11 @@ export class Queue { try { resolve(task()); } catch (err) { - // Reject the returned promise so a synchronously-throwing task fails - // exactly like an asynchronous one whose promise rejects — a caller - // doing rollback in `catch` behaves the same regardless of whether the - // task was declared `async`. The rejection is the single surfacing - // channel for the error: do not also re-throw here, otherwise `run()` - // would report the same error a second time via its `queueMicrotask`. - // Fire-and-forget callers attach their own `.catch()` (see - // `Base/utils.ts` and `ChildrenManager.ts`) so this never becomes an - // unhandled rejection. + // Containing the throw here is what keeps the queue alive: left to + // escape, it would abort `run()` mid-batch, orphan the remaining + // tasks and leave `isScheduled` stuck `true`, wedging the queue for + // good. Rejecting the returned promise also makes a synchronous throw + // behave exactly like an `async` task whose promise rejects. reject(err); } }); @@ -75,10 +71,13 @@ export class Queue { try { this.waiter(() => this.flush()); } catch (err) { - // If the scheduler itself throws, reset the flag before re-throwing so a - // faulty `waiter` can not leave the queue permanently wedged (the flag - // stuck `true` would make every later `scheduleFlush()` early-return and - // never re-arm a flush). + // `waiter` is a public constructor parameter, so a faulty scheduler is + // reachable. Reset the flag before re-throwing: stuck `true`, it would + // make every later `scheduleFlush()` early-return and never re-arm a + // flush. The task `add()` just pushed stays queued on purpose — it runs on + // the next flush that does get scheduled, and its promise never reached + // the caller (the throw pre-empts `add()`'s `return`), so rejecting it + // here could only produce an unobservable rejection. this.isScheduled = false; throw err; } @@ -88,16 +87,11 @@ export class Queue { * Flush current batch. */ flush() { - try { - this.run(this.tasks.splice(0, this.concurrency)); - } finally { - // Always reset the scheduling flag and re-arm the next flush, even when a - // task throws, so a single faulty task can not permanently wedge the queue. - this.isScheduled = false; + this.run(this.tasks.splice(0, this.concurrency)); + this.isScheduled = false; - if (this.tasks.length > 0) { - this.scheduleFlush(); - } + if (this.tasks.length > 0) { + this.scheduleFlush(); } } @@ -108,17 +102,7 @@ export class Queue { let task; // eslint-disable-next-line no-cond-assign while ((task = tasks.shift())) { - try { - task(); - } catch (err) { - // Defence in depth: tasks pushed by `add()` already isolate their own - // errors (they reject the returned promise and never throw here), so - // this only catches an unexpected throw. Keep draining the batch and - // surface the error asynchronously instead of aborting the whole flush. - queueMicrotask(() => { - throw err; - }); - } + task(); } } } diff --git a/packages/js-toolkit/utils/SmartQueue.ts b/packages/js-toolkit/utils/SmartQueue.ts index 6ac0dccb4..1eff6e009 100644 --- a/packages/js-toolkit/utils/SmartQueue.ts +++ b/packages/js-toolkit/utils/SmartQueue.ts @@ -32,16 +32,11 @@ export class SmartQueue extends Queue { * Flush current batch. */ flush() { - try { - this.run(this.tasks); - } finally { - // Always reset the scheduling flag and re-arm the next flush, even when a - // task throws, so a single faulty task can not permanently wedge the queue. - this.isScheduled = false; + this.run(this.tasks); + this.isScheduled = false; - if (this.tasks.length > 0) { - this.scheduleFlush(); - } + if (this.tasks.length > 0) { + this.scheduleFlush(); } } @@ -54,17 +49,7 @@ export class SmartQueue extends Queue { let now = start; // eslint-disable-next-line no-cond-assign while (now - start < LONG_TASK_DURATION && (task = tasks.shift())) { - try { - task(); - } catch (err) { - // Defence in depth: tasks pushed by `add()` already isolate their own - // errors (they reject the returned promise and never throw here), so - // this only catches an unexpected throw. Keep draining the batch and - // surface the error asynchronously instead of aborting the whole flush. - queueMicrotask(() => { - throw err; - }); - } + task(); now = performance.now(); } } diff --git a/packages/tests/utils/Queue.spec.ts b/packages/tests/utils/Queue.spec.ts index 5aaac8de1..df4720677 100644 --- a/packages/tests/utils/Queue.spec.ts +++ b/packages/tests/utils/Queue.spec.ts @@ -91,28 +91,6 @@ describe('The `Queue` class', () => { expect(spy).toHaveBeenCalledTimes(1); }); - it('should reject the promise of a synchronously-throwing task', async () => { - // The re-throw path in `run()` must not fire — the rejection is the single - // surfacing channel, so there is no double report. - const microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => {}); - const queue = new Queue(10); - const error = new Error('boom'); - - const p = queue.add(() => { - throw error; - }); - - // A synchronous throw rejects, exactly like an async task returning a - // rejected promise — the completion contract is consistent. - await expect(p).rejects.toBe(error); - - // The error is surfaced exactly once (through the rejection), not also via - // the global re-throw. - expect(microtask).not.toHaveBeenCalled(); - - microtask.mockRestore(); - }); - it('should reject consistently for sync and async throws', async () => { const queue = new Queue(10); const error = new Error('boom'); @@ -144,4 +122,29 @@ describe('The `Queue` class', () => { // ...but the flag is reset so the queue is not permanently wedged. expect(queue.isScheduled).toBe(false); }); + + it('should keep the pending task queued when the scheduler throws', () => { + let broken = true; + const queue = new Queue(10, (cb) => { + if (broken) { + throw new Error('scheduler boom'); + } + (cb as () => void)(); + }); + const spy = vi.fn(); + + expect(() => queue.add(spy)).toThrow('scheduler boom'); + // The task is not dropped: it is still queued, waiting for a flush that + // does get scheduled. Rejecting it here would be unobservable, since the + // throw pre-empted `add()` returning its promise. + expect(queue.tasks).toHaveLength(1); + expect(spy).not.toHaveBeenCalled(); + + // Once the scheduler works again, the orphaned task runs with the next one. + broken = false; + const next = vi.fn(); + queue.add(next); + expect(spy).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/tests/utils/SmartQueue.spec.ts b/packages/tests/utils/SmartQueue.spec.ts index be39e8370..f8d7edf87 100644 --- a/packages/tests/utils/SmartQueue.spec.ts +++ b/packages/tests/utils/SmartQueue.spec.ts @@ -71,26 +71,26 @@ describe('The `SmartQueue` class', () => { expect(spy).toHaveBeenCalledTimes(1); }); - it('should reject the promise of a synchronously-throwing task', async () => { - // The re-throw path in `run()` must not fire — the rejection is the single - // surfacing channel, so there is no double report. - const microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => {}); + it('should inherit the rejection contract for sync and async throws', async () => { const queue = new SmartQueue(); const error = new Error('boom'); - const p = queue.add(() => { + // `SmartQueue` overrides neither `add()` nor the error isolation: a + // synchronous throw rejects exactly like an `async` task whose promise + // rejects. Observe both rejections right away so the flush below can not + // report them as unhandled. + const sync = queue.add(() => { throw error; }); - await nextTick(); - - // A synchronous throw rejects, exactly like an async task returning a - // rejected promise — the completion contract is consistent. - await expect(p).rejects.toBe(error); + sync.catch(() => {}); + const async = queue.add(async () => { + throw error; + }); + async.catch(() => {}); - // The error is surfaced exactly once (through the rejection), not also via - // the global re-throw. - expect(microtask).not.toHaveBeenCalled(); + await nextTick(); - microtask.mockRestore(); + await expect(sync).rejects.toBe(error); + await expect(async).rejects.toBe(error); }); }); From 9cd3843608a27977a84fa480cacb39a51e91d8ee Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Fri, 7 Aug 2026 10:35:55 +0200 Subject: [PATCH 3/6] Report lifecycle failures on the global error channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed queued lifecycle task was reported with a bare, ungated `console.warn`, which no error monitor observes: not `window.onerror`, not `unhandledrejection`, not the Sentry/Bugsnag defaults. Combined with the lifecycle methods no longer rejecting, a userland exception had no channel at all. Re-throw from a microtask instead — the primitive the queue fix already used — so the failure reaches the global error handler without wedging anything. The thrown error names the instance `$id` and the failed lifecycle, matching the diagnostics style used elsewhere, and carries the original error as its `cause`. In blocking mode `addToQueue` runs the task synchronously in the caller's own stack, so no queue is involved and nothing can wedge: the error is re-thrown there and the lifecycle promise keeps rejecting, which is what `try { await createApp(App, { blocking: true }) } catch` relies on. Adds coverage for the headline scenario (a failing component no longer stops an unrelated one from mounting), for the `$update`, `$destroy` and `$terminate` catches, and for blocking mode. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3 --- packages/js-toolkit/Base/Base.ts | 33 ++- .../Base/managers/ChildrenManager.ts | 11 +- packages/js-toolkit/Base/utils.ts | 54 +++-- packages/tests/Base/Base.spec.ts | 200 ++++++++++++++++-- 4 files changed, 242 insertions(+), 56 deletions(-) diff --git a/packages/js-toolkit/Base/Base.ts b/packages/js-toolkit/Base/Base.ts index 7ff66eb4d..36eeca81e 100644 --- a/packages/js-toolkit/Base/Base.ts +++ b/packages/js-toolkit/Base/Base.ts @@ -7,7 +7,7 @@ import { deleteInstance, addToRegistry, hasInstance, - reportQueuedTaskError, + handleLifecycleError, } from './utils.js'; import { ChildrenManager, @@ -489,15 +489,13 @@ export class Base { this.__callMethod('mounted'); }); } catch (error) { - // A queued mount task failed. Leave the component in an honest state - // instead of emitting `after-mounted` as if init completed: `$isMounted` - // keeps whatever value it reached (still `false` when the failure happened - // during wiring), so the component is not falsely reported as mounted. - // The error is surfaced through the log rather than re-thrown — the queued - // task ran detached, and most callers ($mount is often fire-and-forget) - // would only turn a rejection into an unhandled rejection. + // A queued mount task failed. Do not emit `after-mounted` as if init had + // completed, and leave `$isMounted` at whatever value it reached: `false` + // when the failure happened while wiring, `true` when the `mounted()` hook + // itself threw — the component is wired in that case, and `$destroy()` + // relies on the flag to tear it down. this.__isMounting = false; - reportQueuedTaskError(error); + handleLifecycleError(this, '$mount', error); return this; } @@ -531,10 +529,7 @@ export class Base { await addToQueue(() => this.__callMethod('updated')); } catch (error) { - // Surface a failed queued update task instead of rejecting: `$update` is - // commonly called fire-and-forget, so a rejection would only produce an - // unhandled rejection. - reportQueuedTaskError(error); + handleLifecycleError(this, '$update', error); } return this; @@ -567,10 +562,9 @@ export class Base { await addToQueue(() => this.__callMethod('destroyed')); } catch (error) { // A queued teardown task failed. Do not emit `after-destroyed` (which - // removes the instance from the global storage) as if teardown completed; - // surface the error instead of re-throwing so fire-and-forget callers do - // not produce an unhandled rejection. - reportQueuedTaskError(error); + // removes the instance from the global storage) as if teardown had + // completed. + handleLifecycleError(this, '$destroy', error); return this; } @@ -598,10 +592,7 @@ export class Base { addToQueue(() => this.$el.__base__.set(this.$config.name, 'terminated')), ]); } catch (error) { - // Surface a failed queued termination task instead of rejecting: - // `$terminate` is called fire-and-forget (e.g. from `mutationCallback`), - // so a rejection would only produce an unhandled rejection. - reportQueuedTaskError(error); + handleLifecycleError(this, '$terminate', error); } } diff --git a/packages/js-toolkit/Base/managers/ChildrenManager.ts b/packages/js-toolkit/Base/managers/ChildrenManager.ts index 6a3b836f2..72ce57a3a 100644 --- a/packages/js-toolkit/Base/managers/ChildrenManager.ts +++ b/packages/js-toolkit/Base/managers/ChildrenManager.ts @@ -206,13 +206,18 @@ export class ChildrenManager extends AbstractManager { if (instance instanceof Promise) { // Fire-and-forget: this branch does not push into `promises`, so its // result is never awaited. Attach a `.catch()` so a failure in the - // async child resolution or its queued hook is logged instead of - // becoming an unhandled rejection. + // async child resolution or its queued hook reaches the global error + // handler instead of becoming an unhandled rejection. instance .then((resolvedInstance) => addToQueue(() => this.__triggerHook(hook, resolvedInstance, name)), ) - .catch(reportQueuedTaskError); + .catch((error) => + reportQueuedTaskError( + error, + `[${this.__base.$id}] The \`${hook}\` hook failed for an async \`${name}\` child.`, + ), + ); } else { promises.push(addToQueue(() => this.__triggerHook(hook, instance, name))); } diff --git a/packages/js-toolkit/Base/utils.ts b/packages/js-toolkit/Base/utils.ts index 83b9ef98d..2ce5e506f 100644 --- a/packages/js-toolkit/Base/utils.ts +++ b/packages/js-toolkit/Base/utils.ts @@ -23,18 +23,37 @@ export function addToQueue(fn: () => unknown) { } /** - * Report an error raised by a queued task. + * Surface an error raised by a queued task. * * Queued work runs detached from its original call site — fire-and-forget - * mounts, mutation-driven auto-mounting, async children — so the returned - * promise is often discarded. When such a task fails, its promise rejects with - * no awaiter to observe it; routing it here keeps the failure visible instead - * of turning it into an unhandled rejection or, worse, silently swallowing an - * initialization error. It is reported unconditionally (not gated behind the - * `log` option) so a genuine failure is never lost in production. + * mounts, mutation-driven auto-mounting, async children — so the promise + * returned by `addToQueue` is usually discarded and a rejection would be lost. + * Re-throwing from a microtask puts the failure back on the global error + * channel (`window.onerror`, `uncaughtException`, error monitors) without + * wedging the queue or the caller's control flow. The original error is kept as + * the `cause`, so its stack and type survive; `context` adds the component + * identity a bare rejection can not carry. */ -export function reportQueuedTaskError(error: unknown): void { - console.warn('[@studiometa/js-toolkit] A queued task failed:', error); +export function reportQueuedTaskError(error: unknown, context: string): void { + queueMicrotask(() => { + throw new Error(context, { cause: error }); + }); +} + +/** + * Handle an error raised by a queued lifecycle task. + * + * In blocking mode `addToQueue` runs the task synchronously in the caller's own + * stack — no queue, nothing to wedge — so the error is re-thrown and the + * lifecycle promise rejects exactly as it always has. That keeps + * `try { await createApp(App, { blocking: true }) } catch` working. + */ +export function handleLifecycleError(instance: Base, phase: string, error: unknown): void { + if (features.get('blocking')) { + throw error; + } + + reportQueuedTaskError(error, `[${instance.$id}] The \`${phase}\` lifecycle failed.`); } const selectors = new Map(); @@ -196,10 +215,11 @@ function registry() { } function mutationCallback() { - // Fire-and-forget: the returned promise is discarded, so route a rejection - // (a synchronously-throwing task) to the log instead of letting it become an - // unhandled rejection. `addToQueue` returns `undefined` in blocking mode, - // hence the optional chaining. + // Fire-and-forget: the returned promise is discarded, so route a rejection (a + // constructor throwing while auto-mounting, for instance) to the global error + // handler instead of letting it become an unhandled rejection. In blocking + // mode `addToQueue` runs the task synchronously and returns `undefined` — the + // throw propagates to the mutation observer, hence the optional chaining. addToQueue(() => { for (const [nameOrSelector, ctor] of registry()) { for (const el of getComponentElements(nameOrSelector)) { @@ -208,7 +228,9 @@ function mutationCallback() { } } } - })?.catch(reportQueuedTaskError); + })?.catch((error) => + reportQueuedTaskError(error, 'Auto-mounting components after a DOM mutation failed.'), + ); addToQueue(() => { for (const instance of getInstances()) { @@ -216,7 +238,9 @@ function mutationCallback() { instance.$terminate(); } } - })?.catch(reportQueuedTaskError); + })?.catch((error) => + reportQueuedTaskError(error, 'Auto-terminating components after a DOM mutation failed.'), + ); } export function addToRegistry(nameOrSelector: string, ctor: BaseConstructor) { diff --git a/packages/tests/Base/Base.spec.ts b/packages/tests/Base/Base.spec.ts index 4d5cdd757..9736ac2cd 100644 --- a/packages/tests/Base/Base.spec.ts +++ b/packages/tests/Base/Base.spec.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; import { Base, BaseConfig, @@ -9,7 +9,7 @@ import { withName, } from '@studiometa/js-toolkit'; import { nextTick } from '@studiometa/js-toolkit/utils'; -import { h, mount } from '#test-utils'; +import { h, mount, mockFeatures } from '#test-utils'; let mockedIsDev = true; @@ -740,6 +740,38 @@ describe('A Base instance config', () => { }); describe('The Base lifecycle error boundaries', () => { + let reported: Error[]; + let microtask: MockInstance; + + beforeEach(() => { + reported = []; + // A failed queued lifecycle task is re-thrown from a microtask so it reaches + // `window.onerror` and error monitors. Intercept that channel here, both to + // assert on it and to keep the re-throw from failing the test run. + microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation((callback) => { + try { + callback(); + } catch (error) { + reported.push(error as Error); + } + }); + }); + + afterEach(() => { + microtask.mockRestore(); + }); + + /** + * Let the shared queue drain across a few ticks and give any pending + * rejection the chance to surface. + */ + async function flushQueue() { + for (let i = 0; i < 5; i += 1) { + // eslint-disable-next-line no-await-in-loop + await nextTick(); + } + } + it('should not falsely mark the component mounted when a queued mount task fails', async () => { class Foo extends Base { static config: BaseConfig = { @@ -757,20 +789,136 @@ describe('The Base lifecycle error boundaries', () => { const afterMounted = vi.fn(); foo.$on('after-mounted', afterMounted); - const warn = vi.spyOn(window.console, 'warn').mockImplementation(() => {}); // `$mount` resolves (never rejects) so a fire-and-forget mount can not turn // into an unhandled rejection. await expect(foo.$mount()).resolves.toBe(foo); - // The component is left honestly unmounted... + // The wiring never completed, so the component stays unmounted... expect(foo.$isMounted).toBe(false); // ...`after-mounted` is not emitted as if init had completed... expect(afterMounted).not.toHaveBeenCalled(); - // ...and the failure is surfaced through the log, not swallowed. - expect(warn.mock.calls.some((args) => args.includes(error))).toBe(true); + // ...and the failure reaches the global error handler, carrying the + // component identity and the original error as its cause. + expect(reported).toHaveLength(1); + expect(reported[0].message).toBe(`[${foo.$id}] The \`$mount\` lifecycle failed.`); + expect(reported[0].cause).toBe(error); + }); + + it('should keep mounting unrelated components after one component fails to mount', async () => { + class Failing extends Base { + static config: BaseConfig = { + name: 'Failing', + }; + + mounted() { + throw new Error('mounted boom'); + } + } + + class Working extends Base { + static config: BaseConfig = { + name: 'Working', + }; + } + + const failing = new Failing(h('div')); + const working = new Working(h('div')); + + // Fire-and-forget, like `mutationCallback` does: the failing component's + // tasks are queued on the shared module-level queue first. + failing.$mount(); + // The unrelated component is queued right after, so its tasks land in the + // same batch as the failing one. + const p = working.$mount(); + + await expect(p).resolves.toBe(working); + await flushQueue(); + + // The unrelated component mounted normally — one bad component no longer + // wedges the shared queue and orphans every other component on the page. + expect(working.$isMounted).toBe(true); + // And the queue keeps accepting work afterwards. + const late = new Working(h('div')); + await late.$mount(); + expect(late.$isMounted).toBe(true); + + // The failure was reported once, not swallowed. + expect(reported).toHaveLength(1); + expect(reported[0].message).toBe(`[${failing.$id}] The \`$mount\` lifecycle failed.`); + }); + + it('should not emit `after-destroyed` when the `destroyed` hook throws', async () => { + const error = new Error('destroyed boom'); + + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + + destroyed() { + throw error; + } + } + + const foo = new Foo(h('div')); + await foo.$mount(); + + const afterDestroyed = vi.fn(); + foo.$on('after-destroyed', afterDestroyed); + + await expect(foo.$destroy()).resolves.toBe(foo); + expect(afterDestroyed).not.toHaveBeenCalled(); + + expect(reported).toHaveLength(1); + expect(reported[0].message).toBe(`[${foo.$id}] The \`$destroy\` lifecycle failed.`); + expect(reported[0].cause).toBe(error); + }); + + it('should resolve and report when the `updated` hook throws', async () => { + const error = new Error('updated boom'); - warn.mockRestore(); + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + + updated() { + throw error; + } + } + + const foo = new Foo(h('div')); + await foo.$mount(); + + await expect(foo.$update()).resolves.toBe(foo); + + expect(reported).toHaveLength(1); + expect(reported[0].message).toBe(`[${foo.$id}] The \`$update\` lifecycle failed.`); + expect(reported[0].cause).toBe(error); + }); + + it('should resolve and report when the `terminated` hook throws', async () => { + const error = new Error('terminated boom'); + + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + + terminated() { + throw error; + } + } + + const foo = new Foo(h('div')); + await foo.$mount(); + + await expect(foo.$terminate()).resolves.toBeUndefined(); + + expect(reported).toHaveLength(1); + expect(reported[0].message).toBe(`[${foo.$id}] The \`$terminate\` lifecycle failed.`); + expect(reported[0].cause).toBe(error); }); it('should not produce an unhandled rejection when a fire-and-forget mount fails', async () => { @@ -784,7 +932,6 @@ describe('The Base lifecycle error boundaries', () => { } } - const warn = vi.spyOn(window.console, 'warn').mockImplementation(() => {}); const rejections: unknown[] = []; const onRejection = (reason: unknown) => rejections.push(reason); process.on('unhandledRejection', onRejection); @@ -792,20 +939,39 @@ describe('The Base lifecycle error boundaries', () => { // Fire-and-forget: the returned promise is discarded, like `mutationCallback`. new Foo(h('div')).$mount(); - // Let the queued mount tasks flush across several ticks and give any - // rejection a chance to surface. - for (let i = 0; i < 5; i += 1) { - // eslint-disable-next-line no-await-in-loop - await nextTick(); - } + await flushQueue(); await new Promise((resolve) => setTimeout(resolve, 0)); process.off('unhandledRejection', onRejection); + // No unhandled rejection: the lifecycle promise resolves... expect(rejections).toHaveLength(0); - // The failure is still surfaced through the log. - expect(warn).toHaveBeenCalled(); + // ...but the failure is still visible on the global error channel. + expect(reported).toHaveLength(1); + }); + + it('should keep rejecting in blocking mode', async () => { + const { unmock } = mockFeatures({ blocking: true }); + const error = new Error('mounted boom'); + + class Foo extends Base { + static config: BaseConfig = { + name: 'Foo', + }; + + mounted() { + throw error; + } + } + + const foo = new Foo(h('div')); + + // No queue is involved in blocking mode: the task runs in the caller's own + // stack, so `try { await createApp(App, { blocking: true }) } catch` keeps + // working and the error is not deferred to a microtask. + await expect(foo.$mount()).rejects.toBe(error); + expect(reported).toHaveLength(0); - warn.mockRestore(); + unmock(); }); }); From e68496149409df4123eeb2549b6f3e1570afa3e2 Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Fri, 7 Aug 2026 10:35:55 +0200 Subject: [PATCH 4/6] Keep registerComponent skipping instances that failed to mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `$register()` feeds `$mount()` promises into `Promise.allSettled`, and those no longer reject when a queued mount task fails. `registerComponent` was therefore handing back a never-mounted instance as a successful registration, silently undoing the v3.8.0 skip-and-log behaviour. Check `$isMounted` on the fulfilled branch instead. The rejected branch stays: it still fires when the instance could not even be constructed, which no other channel reports. `registerComponents` is unaffected — `registerComponent` still rejects on a failed dynamic import, which is what its own rejected branch covers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3 --- .../js-toolkit/helpers/registerComponent.ts | 15 ++++++-- .../tests/helpers/registerComponent.spec.ts | 35 +++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/packages/js-toolkit/helpers/registerComponent.ts b/packages/js-toolkit/helpers/registerComponent.ts index f0a09b18d..0ec9c9df0 100644 --- a/packages/js-toolkit/helpers/registerComponent.ts +++ b/packages/js-toolkit/helpers/registerComponent.ts @@ -11,8 +11,10 @@ import { isDev, isFunction } from '../utils/index.js'; * - a factory function returning such a promise (`() => import(...)`). * * Instances are mounted independently: an element that fails to mount is - * skipped (and logged in development) instead of failing the whole call, so - * the resolved array contains every instance that mounted successfully. + * skipped instead of failing the whole call, so the resolved array contains + * every instance that mounted successfully. Mount failures reach the global + * error handler through `$mount`; a constructor that throws is logged in + * development. * * @link https://js-toolkit.studiometa.dev/api/helpers/registerComponent.html * @param ctor The component constructor, or a way to resolve it. @@ -45,9 +47,16 @@ export async function registerComponent { if (result.status === 'fulfilled') { - return [result.value]; + // `$mount()` resolves with the instance even when a queued mount task + // fails — the error goes to the global handler instead of rejecting, so a + // fire-and-forget mount can not become an unhandled rejection. Check the + // flag rather than the promise state to keep the documented contract: + // only instances that actually mounted are returned. + return result.value.$isMounted ? [result.value] : []; } + // The promise still rejects when the instance could not even be + // constructed, which no other channel reports. if (isDev) { console.error( '[registerComponent] An instance failed to mount and was skipped.', diff --git a/packages/tests/helpers/registerComponent.spec.ts b/packages/tests/helpers/registerComponent.spec.ts index ae780ed3e..8e4a7f678 100644 --- a/packages/tests/helpers/registerComponent.spec.ts +++ b/packages/tests/helpers/registerComponent.spec.ts @@ -159,4 +159,39 @@ describe('The `registerComponent` lazy import helper', () => { errorSpy.mockRestore(); }); + + it('should skip an instance whose mount failed', async () => { + const failing = h('div', { dataComponent: 'Component' }); + failing.setAttribute('data-fail', ''); + document.body.append(failing); + + class Component extends Base { + static config = { + name: 'Component', + }; + + constructor(el: HTMLElement) { + super(el); + if (el.hasAttribute('data-fail')) { + // Fail while wiring, so the instance never becomes mounted. + this.__services.enableAll = () => { + throw new Error('boom'); + }; + } + } + } + + // `$mount()` resolves instead of rejecting, so the failure is reported on + // the global error channel. Intercept it here. + const microtask = vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => {}); + + // A never-mounted instance must not be handed back as a successful + // registration. + const instances = await registerComponent(Component); + expect(instances).toHaveLength(1); + expect(instances[0].$el).not.toBe(failing); + expect(instances[0].$isMounted).toBe(true); + + microtask.mockRestore(); + }); }); From 5a4512adf802d5f9b42efdf2e871bbec9f357e64 Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Fri, 7 Aug 2026 10:35:55 +0200 Subject: [PATCH 5/6] Document the lifecycle and queue error contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Queue.add()` now rejects when its task fails, so document it and warn that a task which can fail must have its rejection observed — the page still showed fire-and-forget `queue.add(...)` only. Add an error-handling section to the instance methods reference covering the non-rejecting lifecycle methods, the global error channel, the events that are not emitted on failure, and the blocking-mode exception. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3 --- CHANGELOG.md | 11 +++++++++++ packages/docs/api/instance-methods.md | 10 ++++++++++ packages/docs/utils/Queue.md | 27 +++++++++++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 088f82a4f..214ddfa2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file. The format ## [Unreleased] +### Fixed + +- Fix a component whose lifecycle hook throws synchronously wedging the shared task queue and stopping every other component on the page from mounting — the batch was aborted, its remaining tasks orphaned and the scheduling flag left stuck, so no later batch was ever flushed ([#749](https://github.com/studiometa/js-toolkit/pull/749)) +- Fix `Queue.add()` never settling when its task throws synchronously — the returned promise now rejects with the error, exactly like an `async` task whose promise rejects ([#749](https://github.com/studiometa/js-toolkit/pull/749)) +- Fix `registerComponent()` returning an instance that failed to mount as a successful registration — instances that are not mounted are skipped again, as documented in v3.8.0 ([#749](https://github.com/studiometa/js-toolkit/pull/749)) + +### Changed + +- `$mount()`, `$update()`, `$destroy()` and `$terminate()` no longer reject when a lifecycle hook fails: they run detached on a shared queue and are commonly called fire-and-forget, so a rejection had no awaiter. The error is re-thrown from a microtask instead, keeping it visible to `window.onerror` and error monitors, wrapped with the instance `$id` and the failed lifecycle and carrying the original error as its `cause`. `after-mounted` and `after-destroyed` are still not emitted on failure ([#749](https://github.com/studiometa/js-toolkit/pull/749)) +- With the `blocking` feature enabled the lifecycle methods keep rejecting — no queue is involved there, so `try { await createApp(App, { blocking: true }) } catch {}` still catches startup failures ([#749](https://github.com/studiometa/js-toolkit/pull/749)) + ## [v3.8.2](https://github.com/studiometa/js-toolkit/compare/3.8.1..3.8.2) (2026-08-06) ### Fixed diff --git a/packages/docs/api/instance-methods.md b/packages/docs/api/instance-methods.md index fe01de0d0..52add0aea 100644 --- a/packages/docs/api/instance-methods.md +++ b/packages/docs/api/instance-methods.md @@ -277,3 +277,13 @@ Terminate the component. Its instance becomes available for garbage collection. :::warning A terminated component can not be re-mounted, use with precaution. ::: + +## Error handling + +Lifecycle methods run their work through a shared task queue, and they are commonly called fire-and-forget (`$mount()` from the auto-mounting mutation observer, `$terminate()` when an element leaves the DOM). A rejection there would have no awaiter, so `$mount()`, `$update()`, `$destroy()` and `$terminate()` **resolve even when a lifecycle hook throws**. + +The error is not swallowed: it is re-thrown from a microtask, so it reaches `window.onerror` and any error monitor, wrapped in an error naming the instance and the failed lifecycle, with the original error as its `cause`. + +A failed lifecycle stops at the point of failure: `after-mounted` and `after-destroyed` are not emitted, and a component whose wiring failed stays unmounted. A component whose `mounted()` hook threw is wired, so `$isMounted` is `true` and `$destroy()` can still tear it down. + +With the [`blocking` feature](./helpers/createApp.md) enabled no queue is involved — the work runs synchronously in the caller's stack — so the lifecycle methods keep rejecting and `try { await createApp(App, { blocking: true }) } catch {}` still catches startup failures. diff --git a/packages/docs/utils/Queue.md b/packages/docs/utils/Queue.md index dcf39fb04..e37c8512f 100644 --- a/packages/docs/utils/Queue.md +++ b/packages/docs/utils/Queue.md @@ -17,3 +17,30 @@ queue.add(() => console.log('2')); - `concurrency` (`Number`): the number of tasks to execute at the same time, defaults to `10` - `waiter` (`(cb: (...args:unknown[]) => unknown) => unknown`): a scheduler function for the next batch execution, defaults to an immediately invoked function `(cb) => cb()` + +## `add(task)` + +Add a task to the queue. + +**Return value** + +- `Promise`: a promise resolving with the value returned by the task. + +The promise **rejects when the task fails**, whether it throws synchronously or returns a rejected promise. The failure is contained: the other tasks of the batch still run, and the queue keeps scheduling later batches. + +:::warning +A task that can fail must have its rejection observed, otherwise it becomes an unhandled rejection. + +```js +// Fire-and-forget is fine for a task that can not fail. +queue.add(() => console.log('1')); + +// Otherwise, await the promise or attach a `catch`. +try { + await queue.add(() => mightThrow()); +} catch (error) { + // Handle the failure. +} +``` + +::: From 5d6ec2f1c44c6cb56a6154b4bceccc75dc68d08a Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Fri, 7 Aug 2026 10:37:23 +0200 Subject: [PATCH 6/6] Restore mocked features from afterEach in the lifecycle tests Match the pattern used in parent-resolution-residual.spec.ts so a failed assertion cannot leave `blocking` enabled for later tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3 --- packages/tests/Base/Base.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/tests/Base/Base.spec.ts b/packages/tests/Base/Base.spec.ts index 9736ac2cd..258e7892d 100644 --- a/packages/tests/Base/Base.spec.ts +++ b/packages/tests/Base/Base.spec.ts @@ -742,6 +742,7 @@ describe('A Base instance config', () => { describe('The Base lifecycle error boundaries', () => { let reported: Error[]; let microtask: MockInstance; + let restoreFeatures: (() => void) | undefined; beforeEach(() => { reported = []; @@ -759,6 +760,8 @@ describe('The Base lifecycle error boundaries', () => { afterEach(() => { microtask.mockRestore(); + restoreFeatures?.(); + restoreFeatures = undefined; }); /** @@ -951,7 +954,7 @@ describe('The Base lifecycle error boundaries', () => { }); it('should keep rejecting in blocking mode', async () => { - const { unmock } = mockFeatures({ blocking: true }); + restoreFeatures = mockFeatures({ blocking: true }).unmock; const error = new Error('mounted boom'); class Foo extends Base { @@ -971,7 +974,5 @@ describe('The Base lifecycle error boundaries', () => { // working and the error is not deferred to a microtask. await expect(foo.$mount()).rejects.toBe(error); expect(reported).toHaveLength(0); - - unmock(); }); });