diff --git a/src/index.ts b/src/index.ts index fbf887c..800fe53 100644 --- a/src/index.ts +++ b/src/index.ts @@ -18,6 +18,12 @@ interface SignalNode extends ReactiveNode { pendingValue: T; } +// Marks a parent (effect or scope) whose deps include at least one child +// effect. Used to gate the dispose-children-first slow path in run() so +// leaf effects (no children, no own cleanup) avoid the extra deps walk. +// The bit is outside ReactiveFlags' range and never touched by system.ts. +const HasChildEffect = 64; + let cycle = 0; let runDepth = 0; let batchDepth = 0; @@ -64,13 +70,21 @@ const { queued[insertIndex] = left; } }, - unwatched(node) { - if (!(node.flags & ReactiveFlags.Mutable)) { + unwatched(node: SignalNode | ComputedNode | EffectNode | EffectScopeNode) { + if ('getter' in node) { + if (node.depsTail !== undefined) { + node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; + disposeAllDepsInReverse(node); + } + } + else if ('currentValue' in node) { + // Nothing to do for signals, they are always mutable and never dirty until pendingValue changes + } + else if ('fn' in node) { + effectOper.call(node); + } + else { effectScopeOper.call(node); - } else if (node.depsTail !== undefined) { - node.depsTail = undefined; - node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; - purgeDeps(node); } }, }); @@ -161,6 +175,7 @@ export function effect(fn: () => void | (() => void)): () => void { const prevSub = setActiveSub(e); if (prevSub !== undefined) { link(e, prevSub, 0); + prevSub.flags |= HasChildEffect; } try { ++runDepth; @@ -184,6 +199,7 @@ export function effectScope(fn: () => void): () => void { const prevSub = setActiveSub(e); if (prevSub !== undefined) { link(e, prevSub, 0); + prevSub.flags |= HasChildEffect; } try { fn(); @@ -222,6 +238,17 @@ export function trigger(fn: () => void) { } function updateComputed(c: ComputedNode): boolean { + if (c.flags & HasChildEffect) { + let link = c.depsTail; + while (link !== undefined) { + const prev = link.prevDep; + const dep = link.dep; + if (!('getter' in dep) && !('currentValue' in dep)) { + unlink(link, c); + } + link = prev; + } + } c.depsTail = undefined; c.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck; const prevSub = setActiveSub(c); @@ -250,6 +277,17 @@ function run(e: EffectNode): void { && checkDirty(e.deps!, e) ) ) { + if (flags & HasChildEffect) { + let link = e.depsTail; + while (link !== undefined) { + const prev = link.prevDep; + const dep = link.dep; + if (!('getter' in dep) && !('currentValue' in dep)) { + unlink(link, e); + } + link = prev; + } + } if (e.cleanup) { runCleanup(e); if (!e.flags) { @@ -270,7 +308,7 @@ function run(e: EffectNode): void { purgeDeps(e); } } else if (e.deps !== undefined) { - e.flags = ReactiveFlags.Watching; + e.flags = ReactiveFlags.Watching | (flags & HasChildEffect); } } @@ -369,22 +407,30 @@ function runCleanup(e: EffectNode): void { } function effectOper(this: EffectNode): void { + effectScopeOper.call(this); if (this.cleanup) { runCleanup(this); } - effectScopeOper.call(this); } function effectScopeOper(this: EffectScopeNode): void { - this.depsTail = undefined; this.flags = ReactiveFlags.None; - purgeDeps(this); + disposeAllDepsInReverse(this); const sub = this.subs; if (sub !== undefined) { unlink(sub); } } +function disposeAllDepsInReverse(sub: ReactiveNode): void { + let link = sub.depsTail; + while (link !== undefined) { + const prev = link.prevDep; + unlink(link, sub); + link = prev; + } +} + function purgeDeps(sub: ReactiveNode) { const depsTail = sub.depsTail; let dep = depsTail !== undefined ? depsTail.nextDep : sub.deps; diff --git a/tests/effect.spec.ts b/tests/effect.spec.ts index 319d257..98f6a01 100644 --- a/tests/effect.spec.ts +++ b/tests/effect.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from 'vitest'; -import { effect, getActiveSub, signal } from '../src'; +import { computed, effect, getActiveSub, signal } from '../src'; import { ReactiveFlags } from '../src/system'; test('should support custom recurse effect', () => { @@ -16,6 +16,197 @@ test('should support custom recurse effect', () => { expect(triggers).toBe(6); }); +test('cleanup order on outer re-run: inner before outer, before new run', () => { + const log: string[] = []; + const a = signal(0); + + effect(() => { + a(); + log.push('outer:run'); + effect(() => { + log.push('inner:run'); + return () => log.push('inner:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + expect(log).toEqual(['outer:run', 'inner:run']); + + log.length = 0; + a(1); + expect(log).toEqual([ + 'inner:cleanup', + 'outer:cleanup', + 'outer:run', + 'inner:run', + ]); +}); + +test('cleanup order on dispose: inner before outer', () => { + const log: string[] = []; + + const dispose = effect(() => { + log.push('outer:run'); + effect(() => { + log.push('inner:run'); + return () => log.push('inner:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + log.length = 0; + dispose(); + expect(log).toEqual(['inner:cleanup', 'outer:cleanup']); +}); + +test('sibling cleanup order on dispose: reverse creation (LIFO)', () => { + const log: string[] = []; + + const dispose = effect(() => { + effect(() => { + return () => log.push('inner1:cleanup'); + }); + effect(() => { + return () => log.push('inner2:cleanup'); + }); + effect(() => { + return () => log.push('inner3:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + dispose(); + expect(log).toEqual([ + 'inner3:cleanup', + 'inner2:cleanup', + 'inner1:cleanup', + 'outer:cleanup', + ]); +}); + +test('sibling cleanup order on outer re-run: reverse creation (LIFO)', () => { + const log: string[] = []; + const a = signal(0); + + effect(() => { + a(); + effect(() => { + return () => log.push('inner1:cleanup'); + }); + effect(() => { + return () => log.push('inner2:cleanup'); + }); + effect(() => { + return () => log.push('inner3:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + log.length = 0; + + a(1); + expect(log.slice(0, 4)).toEqual([ + 'inner3:cleanup', + 'inner2:cleanup', + 'inner1:cleanup', + 'outer:cleanup', + ]); +}); + +test('three-level nested cleanup on dispose: deepest first (depth-first reverse)', () => { + const log: string[] = []; + + const dispose = effect(() => { + effect(() => { + effect(() => { + return () => log.push('grandchild:cleanup'); + }); + return () => log.push('child:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + dispose(); + expect(log).toEqual([ + 'grandchild:cleanup', + 'child:cleanup', + 'outer:cleanup', + ]); +}); + +test('computed unwatched: child effect cleanups run in reverse creation (LIFO)', () => { + // When the computed loses its last subscriber and gets unwatched, any + // effects it created during its getter must be cleaned up LIFO, not FIFO. + const log: string[] = []; + const c = computed(() => { + effect(() => () => log.push('e1')); + effect(() => () => log.push('e2')); + effect(() => () => log.push('e3')); + return 0; + }); + const dispose = effect(() => { c(); }); + log.length = 0; + dispose(); + expect(log).toEqual(['e3', 'e2', 'e1']); +}); + +test('effect created inside computed: old inner cleanup runs before new inner setup', () => { + // When a computed re-evaluates, any effects its getter created on the + // previous run must be disposed (with cleanup) before the getter runs + // again. Otherwise the old inner's cleanup ends up running after the + // new inner has already been set up. + const a = signal(0); + const log: string[] = []; + + const c = computed(() => { + log.push('computed:eval'); + effect(() => { + log.push('inner:run'); + return () => log.push('inner:cleanup'); + }); + return a(); + }); + + effect(() => { + c(); + }); + log.length = 0; + + a(1); + expect(log).toEqual([ + 'inner:cleanup', + 'computed:eval', + 'inner:run', + ]); +}); + +test('cleanup order is correct on outer re-run after a prior inner-only re-run', () => { + // Regression: inner re-running alone routes outer through run()'s + // not-dirty branch (restore Watching), which must preserve any + // "has child effect" tracking so the next real outer re-run still + // disposes children before its own cleanup. + const a = signal(0); + const b = signal(0); + const log: string[] = []; + + effect(() => { + a(); + log.push('outer:run'); + effect(() => { + b(); + log.push('inner:run'); + return () => log.push('inner:cleanup'); + }); + return () => log.push('outer:cleanup'); + }); + + b(1); // inner re-runs alone; outer is touched via notify chain + log.length = 0; + + a(1); + expect(log).toEqual([ + 'inner:cleanup', + 'outer:cleanup', + 'outer:run', + 'inner:run', + ]); +}); + // https://github.com/stackblitz/alien-signals/issues/115 test('outer effect keeps responding to its own dep after inner re-runs', () => { const a = signal(0); diff --git a/tests/effectScope.spec.ts b/tests/effectScope.spec.ts new file mode 100644 index 0000000..8746cf8 --- /dev/null +++ b/tests/effectScope.spec.ts @@ -0,0 +1,73 @@ +import { expect, test } from 'vitest'; +import { effect, effectScope, signal } from '../src'; + +test('scope dispose runs child effect cleanup', () => { + const log: string[] = []; + const dispose = effectScope(() => { + effect(() => { + return () => log.push('inner:cleanup'); + }); + }); + dispose(); + expect(log).toEqual(['inner:cleanup']); +}); + +test('scope dispose: sibling effects clean up in reverse creation (LIFO)', () => { + const log: string[] = []; + const dispose = effectScope(() => { + effect(() => { + return () => log.push('e1:cleanup'); + }); + effect(() => { + return () => log.push('e2:cleanup'); + }); + effect(() => { + return () => log.push('e3:cleanup'); + }); + }); + dispose(); + expect(log).toEqual(['e3:cleanup', 'e2:cleanup', 'e1:cleanup']); +}); + +test('scope dispose: nested effect cleanup runs depth-first reverse', () => { + const log: string[] = []; + const dispose = effectScope(() => { + effect(() => { + effect(() => { + return () => log.push('grandchild:cleanup'); + }); + return () => log.push('child:cleanup'); + }); + }); + dispose(); + expect(log).toEqual(['grandchild:cleanup', 'child:cleanup']); +}); + +test('scope as intermediate parent: cleanup order respects nesting', () => { + // When effectScope is used as an intermediate scope inside an outer + // effect, the outer's re-run must still dispose the scope (and its + // effects) before running the outer's own cleanup. + const a = signal(0); + const log: string[] = []; + + effect(() => { + a(); + log.push('outer:run'); + effectScope(() => { + effect(() => { + log.push('inner:run'); + return () => log.push('inner:cleanup'); + }); + }); + return () => log.push('outer:cleanup'); + }); + log.length = 0; + + a(1); + expect(log).toEqual([ + 'inner:cleanup', + 'outer:cleanup', + 'outer:run', + 'inner:run', + ]); +});