diff --git a/src/index.ts b/src/index.ts index d503438..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 { @@ -99,6 +110,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; } @@ -153,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, @@ -173,13 +189,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 +216,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; } @@ -256,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; @@ -333,36 +362,52 @@ 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(); + 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; } - } 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) { - link(this, sub, cycle); - } return this.value!; } @@ -388,7 +433,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; diff --git a/tests/computed-throw.spec.ts b/tests/computed-throw.spec.ts new file mode 100644 index 0000000..a1b5520 --- /dev/null +++ b/tests/computed-throw.spec.ts @@ -0,0 +1,72 @@ +import { expect, test } from 'vitest'; +import { computed, effect, signal } from '../src'; + +/** + * 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 re-throws on unrelated re-runs (not stale undefined)', () => { + const s = signal(0); + const other = 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++; + other(); // subscribe to an unrelated signal + try { + effectValue = c(); + } catch (e) { + effectValue = 'threw'; + } + }); + + 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(); +}); + +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; + }); + + // 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); +}); 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); +});