From a0f82d85cc1690c52d8e30b28b25da210ddefc80 Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:48:00 -0600 Subject: [PATCH 1/6] test: effect/effectScope teardown repro (issue #118) - stopped effects, failed setup leaks --- tests/effect-teardown-repro.spec.ts | 67 +++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/effect-teardown-repro.spec.ts diff --git a/tests/effect-teardown-repro.spec.ts b/tests/effect-teardown-repro.spec.ts new file mode 100644 index 0000000..2820d84 --- /dev/null +++ b/tests/effect-teardown-repro.spec.ts @@ -0,0 +1,67 @@ +import { expect, test } from 'vitest'; +import { effect, effectScope, getActiveSub, signal } from '../src'; +import { ReactiveFlags, type ReactiveNode } from '../src/system'; + +test('stopped effect does not subscribe to signals read later in the same run', () => { + const rerun = signal(0); + const readAfterStop = signal(0); + let stop!: () => void; + let node: ReactiveNode | undefined; + let stopDuringRun = false; + let runs = 0; + + stop = effect(() => { + node ??= getActiveSub(); + runs++; + rerun(); + if (stopDuringRun) { + stop(); + readAfterStop(); + } + }); + + expect(runs).toBe(1); + + stopDuringRun = true; + rerun(1); + + expect(runs).toBe(2); + expect(node!.flags).toBe(ReactiveFlags.None); + expect(node!.deps).toBeUndefined(); +}); + +test('failed effect setup does not leave a live subscription behind', () => { + const source = signal(0); + let runs = 0; + + expect(() => + effect(() => { + runs++; + source(); + throw new Error('setup failed'); + }) + ).toThrow('setup failed'); + + expect(runs).toBe(1); + expect(() => source(1)).not.toThrow(); + expect(runs).toBe(1); +}); + +test('failed effect scope setup disposes child effects created before throw', () => { + const source = signal(0); + let childRuns = 0; + + expect(() => + effectScope(() => { + effect(() => { + childRuns++; + source(); + }); + throw new Error('scope setup failed'); + }) + ).toThrow('scope setup failed'); + + expect(childRuns).toBe(1); + source(1); + expect(childRuns).toBe(1); +}); From d418051af53561c705bc9d8ea1271189a828ffbf Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:48:00 -0600 Subject: [PATCH 2/6] fix: effect/effectScope teardown on failed setup + shouldTrack guard (issue #118/#119) --- src/index.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index d503438..42850b6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -99,6 +99,10 @@ export function setActiveSub(sub?: ReactiveNode) { return prevSub; } +function shouldTrack(sub: ReactiveNode): boolean { + return !!(sub.flags & (ReactiveFlags.Mutable | ReactiveFlags.Watching)); +} + export function getBatchDepth(): number { return batchDepth; } @@ -173,13 +177,16 @@ export function effect(fn: () => void | (() => void)): () => void { flags: ReactiveFlags.Watching | ReactiveFlags.RecursedCheck, }; const prevSub = setActiveSub(e); - if (prevSub !== undefined) { + if (prevSub !== undefined && shouldTrack(prevSub)) { link(e, prevSub, 0); prevSub.flags |= HasChildEffect; } try { ++runDepth; e.cleanup = e.fn(); + } catch (error) { + effectOper.call(e); + throw error; } finally { --runDepth; activeSub = prevSub; @@ -197,12 +204,15 @@ export function effectScope(fn: () => void): () => void { flags: ReactiveFlags.Mutable, }; const prevSub = setActiveSub(e); - if (prevSub !== undefined) { + if (prevSub !== undefined && shouldTrack(prevSub)) { link(e, prevSub, 0); prevSub.flags |= HasChildEffect; } try { fn(); + } catch (error) { + effectScopeOper.call(e); + throw error; } finally { activeSub = prevSub; } @@ -360,7 +370,7 @@ function computedOper(this: ComputedNode): T { } } const sub = activeSub; - if (sub !== undefined) { + if (sub !== undefined && shouldTrack(sub)) { link(this, sub, cycle); } return this.value!; @@ -388,7 +398,7 @@ function signalOper(this: SignalNode, ...value: [T]): T | void { } } const sub = activeSub; - if (sub !== undefined) { + if (sub !== undefined && shouldTrack(sub)) { link(this, sub, cycle); } return this.currentValue; From 5de9b219e189e999587969bb3ec62c081d4cb855 Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:35:04 -0600 Subject: [PATCH 3/6] test: computed-throw original issue - computed whose getter throws on first eval recovers after dep change (link-in-finally fix) --- tests/computed-throw.spec.ts | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tests/computed-throw.spec.ts diff --git a/tests/computed-throw.spec.ts b/tests/computed-throw.spec.ts new file mode 100644 index 0000000..a116233 --- /dev/null +++ b/tests/computed-throw.spec.ts @@ -0,0 +1,50 @@ +import { expect, test } from 'vitest'; +import { computed, effect, signal } from '../src'; + +/** + * Regression test: a computed whose getter throws on FIRST evaluation must + * still be linked to its subscriber, so it re-runs once its dependencies + * change and the getter can succeed. + * + * Before the fix, `computedOper` linked the computed to the active subscriber + * AFTER running the getter; a throwing getter skipped the link entirely, + * leaving the computed permanently stale (never re-evaluated, never + * propagated). This is the computed counterpart of the effect/effectScope + * teardown issues in #118. + */ +test('computed that throws on first eval recovers after dependency change', () => { + const s = signal(0); + let getterCalls = 0; + const c = computed(() => { + getterCalls++; + if (s() === 0) { + throw new Error('PENDING'); + } + return s() * 2; + }); + + let effectRuns = 0; + let effectValue: unknown = 'unset'; + const stop = effect(() => { + effectRuns++; + try { + effectValue = c(); + } catch (e) { + effectValue = 'threw'; + } + }); + + // First run: getter throws, effect catches it. + expect(effectRuns).toBe(1); + expect(effectValue).toBe('threw'); + expect(getterCalls).toBe(1); + + // Change the dependency so the getter would now succeed. + s(5); + + // The effect must re-run and read the new value. + expect(effectRuns).toBe(2); + expect(effectValue).toBe(10); + expect(getterCalls).toBe(2); + stop(); +}); From ef66787c3f3b1018b1e1a4e3f6957a989ea58f91 Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:59:53 -0600 Subject: [PATCH 4/6] fix: computedOper link-in-finally - computed whose getter throws on first eval still links to subscriber (computed-throw issue) --- src/index.ts | 54 ++++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/index.ts b/src/index.ts index 42850b6..958ba1f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -343,36 +343,40 @@ function flush(): void { function computedOper(this: ComputedNode): T { const flags = this.flags; - if ( - flags & ReactiveFlags.Dirty - || ( - flags & ReactiveFlags.Pending - && ( - checkDirty(this.deps!, this) - || (this.flags = flags & ~ReactiveFlags.Pending, false) + const sub = activeSub; + const shouldLink = sub !== undefined && shouldTrack(sub); + try { + if ( + flags & ReactiveFlags.Dirty + || ( + flags & ReactiveFlags.Pending + && ( + checkDirty(this.deps!, this) + || (this.flags = flags & ~ReactiveFlags.Pending, false) + ) ) - ) - ) { - if (updateComputed(this)) { - const subs = this.subs; - if (subs !== undefined) { - shallowPropagate(subs); + ) { + if (updateComputed(this)) { + const subs = this.subs; + if (subs !== undefined) { + shallowPropagate(subs); + } + } + } else if (!flags) { + this.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck; + const prevSub = setActiveSub(this); + try { + this.value = this.getter(); + } finally { + activeSub = prevSub; + this.flags &= ~ReactiveFlags.RecursedCheck; } } - } else if (!flags) { - this.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck; - const prevSub = setActiveSub(this); - try { - this.value = this.getter(); - } finally { - activeSub = prevSub; - this.flags &= ~ReactiveFlags.RecursedCheck; + } finally { + if (shouldLink) { + link(this, sub, cycle); } } - const sub = activeSub; - if (sub !== undefined && shouldTrack(sub)) { - link(this, sub, cycle); - } return this.value!; } From 4a776190cfaa94efcbbe6736e08ca76262edc6c5 Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:50:42 -0600 Subject: [PATCH 5/6] test: computed-throw stale-undefined edge case - re-throws stored error on unrelated re-runs and direct re-read --- tests/computed-throw.spec.ts | 56 +++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/tests/computed-throw.spec.ts b/tests/computed-throw.spec.ts index a116233..a1b5520 100644 --- a/tests/computed-throw.spec.ts +++ b/tests/computed-throw.spec.ts @@ -2,18 +2,15 @@ import { expect, test } from 'vitest'; import { computed, effect, signal } from '../src'; /** - * Regression test: a computed whose getter throws on FIRST evaluation must - * still be linked to its subscriber, so it re-runs once its dependencies - * change and the getter can succeed. - * - * Before the fix, `computedOper` linked the computed to the active subscriber - * AFTER running the getter; a throwing getter skipped the link entirely, - * leaving the computed permanently stale (never re-evaluated, never - * propagated). This is the computed counterpart of the effect/effectScope - * teardown issues in #118. + * Regression test: after a computed's getter throws on first evaluation, the + * computed is left in a cached `Mutable` state with `value` unset. A + * subsequent read (e.g. from an unrelated re-run of the subscriber, or a + * direct re-read) must re-throw the stored error rather than return the stale + * `undefined` value. */ -test('computed that throws on first eval recovers after dependency change', () => { +test('computed that throws on first eval re-throws on unrelated re-runs (not stale undefined)', () => { const s = signal(0); + const other = signal(0); let getterCalls = 0; const c = computed(() => { getterCalls++; @@ -27,6 +24,7 @@ test('computed that throws on first eval recovers after dependency change', () = let effectValue: unknown = 'unset'; const stop = effect(() => { effectRuns++; + other(); // subscribe to an unrelated signal try { effectValue = c(); } catch (e) { @@ -34,17 +32,41 @@ test('computed that throws on first eval recovers after dependency change', () = } }); - // First run: getter throws, effect catches it. expect(effectRuns).toBe(1); expect(effectValue).toBe('threw'); + + // Writing an unrelated signal re-runs the effect, but the computed's own + // deps haven't changed, so the getter is NOT re-invoked. The stored error + // must be re-thrown (not a stale `undefined` returned). + other(1); + expect(effectRuns).toBe(2); expect(getterCalls).toBe(1); + expect(effectValue).toBe('threw'); + stop(); +}); - // Change the dependency so the getter would now succeed. - s(5); +test('computed that throws on first eval re-throws on direct re-read (not stale undefined)', () => { + const s = signal(0); + let getterCalls = 0; + const c = computed(() => { + getterCalls++; + if (s() === 0) { + throw new Error('PENDING'); + } + return s() * 2; + }); - // The effect must re-run and read the new value. - expect(effectRuns).toBe(2); - expect(effectValue).toBe(10); + // First read throws. + expect(() => c()).toThrow('PENDING'); + expect(getterCalls).toBe(1); + + // A direct re-read with unchanged deps must re-throw the stored error, + // not return the stale cached `undefined`. + expect(() => c()).toThrow('PENDING'); + expect(getterCalls).toBe(1); + + // Once the dep changes, the getter re-runs and succeeds. + s(5); + expect(c()).toBe(10); expect(getterCalls).toBe(2); - stop(); }); From d9a310adfa4dde2644976a8c0f7718a497c3cff0 Mon Sep 17 00:00:00 2001 From: Nick Chomey <88559987+nickchomey@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:35:04 -0600 Subject: [PATCH 6/6] fix: computed that throws on first eval re-throws stored error instead of stale undefined --- src/index.ts | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index 958ba1f..aef51d8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -11,6 +11,17 @@ interface EffectNode extends ReactiveNode { interface ComputedNode extends ReactiveNode { value: T | undefined; getter: (previousValue?: T) => T; + /** + * The error thrown by the most recent getter evaluation, if any. + * + * When a computed's getter throws, the computed is left in a cached + * `Mutable` state with `value` unset. Without this field, a subsequent + * read (e.g. from an unrelated re-run of the subscriber) would fall + * through all branches and return the stale `undefined` instead of + * surfacing the error. Storing it lets `computedOper` re-throw the same + * error on reads until the dependencies change and the getter re-runs. + */ + error: unknown; } interface SignalNode extends ReactiveNode { @@ -157,6 +168,7 @@ export function signal(initialValue?: T): { export function computed(getter: (previousValue?: T) => T): () => T { return computedOper.bind({ value: undefined, + error: undefined, subs: undefined, subsTail: undefined, deps: undefined, @@ -266,7 +278,14 @@ function updateComputed(c: ComputedNode): boolean { try { ++cycle; const oldValue = c.value; - return oldValue !== (c.value = c.getter(oldValue)); + const newValue = c.getter(oldValue); + c.error = undefined; + return oldValue !== (c.value = newValue); + } catch (error) { + // Store the error so subsequent reads (before the deps change) can + // re-throw it instead of returning the stale cached value. + c.error = error; + throw error; } finally { activeSub = prevSub; c.flags &= ~ReactiveFlags.RecursedCheck; @@ -367,10 +386,22 @@ function computedOper(this: ComputedNode): T { const prevSub = setActiveSub(this); try { this.value = this.getter(); + this.error = undefined; + } catch (error) { + // Store the error so subsequent reads (before the deps + // change) can re-throw it instead of returning the stale + // cached value. + this.error = error; + throw error; } finally { activeSub = prevSub; this.flags &= ~ReactiveFlags.RecursedCheck; } + } else if (this.error !== undefined) { + // Cached but the last evaluation threw, and the deps haven't + // changed (not Dirty/Pending): re-throw the stored error rather + // than returning the stale `undefined` value. + throw this.error; } } finally { if (shouldLink) {