From 6bf679b352575c2e5e1370225793caf6fb468fc2 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Thu, 14 May 2026 21:39:45 +0800 Subject: [PATCH 1/5] fix: dispose nested effects in reverse-creation order, before own cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in cleanup of nested effects: 1. When an inner effect is auto-disposed (parent re-run / parent dispose / scope dispose), its cleanup was never called. The unwatched callback routed straight to effectScopeOper, which clears state but never reads .cleanup. Inner cleanup was silently dropped on every auto-dispose path. 2. Even after wiring cleanup into the auto-dispose path, the order was inverted: the parent's own cleanup ran first (in effectOper / run() / updateComputed), then children were disposed via purgeDeps at the end. Standard reverse-of-creation order needs the inner cleanup to run BEFORE the outer cleanup, so the inner can still rely on whatever state the outer set up; and BEFORE the body re-runs (in the re-run case), so the old inner doesn't end up cleaning up state that the new inner already set up. Fixes: - unwatched() dispatches by node shape: EffectNode → effectOper (full dispose including cleanup), ComputedNode → its existing stale-clear path, EffectScopeNode → effectScopeOper. - effectOper / effectScopeOper / run() / updateComputed all walk this.depsTail backward and unlink child EffectNode deps before the node's own cleanup runs. unlink triggers unwatched → recursive effectOper on each child, so grandchildren get cleaned up depth-first reverse before their parent. - A per-effect flag (HasChildEffect, defined locally in index.ts — outside ReactiveFlags' range so system.ts never touches it) gates the pre-walk in run() and updateComputed. Leaf effects with no child effects skip the walk entirely; the flag is set by effect() when it links to a parent and is naturally cleared at the start of a body re-run. - The not-dirty branch of run() (restore Watching after notify chain) explicitly preserves HasChildEffect: e.flags = Watching | (flags & HasChildEffect). Otherwise an inner-only re-run would clobber the outer's bit and the next real re-run would skip the pre-walk. Tests: - effect.spec.ts: - cleanup order on outer re-run - cleanup order on dispose - sibling cleanup on dispose / re-run (LIFO) - three-level nested cleanup - cleanup order after a prior inner-only re-run - effect created inside computed (parent is ComputedNode) - effectScope.spec.ts: - scope dispose runs child cleanup - sibling LIFO on scope dispose - depth-first reverse for nested in scope All 198 pass. --- src/index.ts | 60 ++++++++++--- tests/effect.spec.ts | 177 +++++++++++++++++++++++++++++++++++++- tests/effectScope.spec.ts | 44 ++++++++++ 3 files changed, 270 insertions(+), 11 deletions(-) create mode 100644 tests/effectScope.spec.ts diff --git a/src/index.ts b/src/index.ts index fbf887c..f197700 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,22 @@ const { queued[insertIndex] = left; } }, - unwatched(node) { - if (!(node.flags & ReactiveFlags.Mutable)) { + unwatched(node: SignalNode | ComputedNode | EffectNode | EffectScopeNode) { + if ('fn' in node) { + effectOper.call(node); + } + else if ('getter' in node) { + if (node.depsTail !== undefined) { + node.depsTail = undefined; + node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; + purgeDeps(node); + } + } + else if ('currentValue' in node) { + // Nothing to do for signals, they are always mutable and never dirty until pendingValue changes + } + else { effectScopeOper.call(node); - } else if (node.depsTail !== undefined) { - node.depsTail = undefined; - node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; - purgeDeps(node); } }, }); @@ -161,6 +176,7 @@ export function effect(fn: () => void | (() => void)): () => void { const prevSub = setActiveSub(e); if (prevSub !== undefined) { link(e, prevSub, 0); + prevSub.flags |= HasChildEffect; } try { ++runDepth; @@ -222,6 +238,16 @@ 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; + if ('fn' in link.dep) { + unlink(link, c); + } + link = prev; + } + } c.depsTail = undefined; c.flags = ReactiveFlags.Mutable | ReactiveFlags.RecursedCheck; const prevSub = setActiveSub(c); @@ -250,6 +276,16 @@ function run(e: EffectNode): void { && checkDirty(e.deps!, e) ) ) { + if (flags & HasChildEffect) { + let link = e.depsTail; + while (link !== undefined) { + const prev = link.prevDep; + if ('fn' in link.dep) { + unlink(link, e); + } + link = prev; + } + } if (e.cleanup) { runCleanup(e); if (!e.flags) { @@ -270,7 +306,7 @@ function run(e: EffectNode): void { purgeDeps(e); } } else if (e.deps !== undefined) { - e.flags = ReactiveFlags.Watching; + e.flags = ReactiveFlags.Watching | (flags & HasChildEffect); } } @@ -369,16 +405,20 @@ 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); + let link = this.depsTail; + while (link !== undefined) { + const prev = link.prevDep; + unlink(link, this); + link = prev; + } const sub = this.subs; if (sub !== undefined) { unlink(sub); diff --git a/tests/effect.spec.ts b/tests/effect.spec.ts index 319d257..3f6253f 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,181 @@ 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('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..5bb0880 --- /dev/null +++ b/tests/effectScope.spec.ts @@ -0,0 +1,44 @@ +import { expect, test } from 'vitest'; +import { effect, effectScope } 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']); +}); From e8d279140281f9fd1f7f753ecfd09b66bd3a9e6e Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Thu, 14 May 2026 22:03:25 +0800 Subject: [PATCH 2/5] fix two cleanup-order gaps caught by ultrareview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 002 — effectScope as intermediate parent: effectScope() linked itself to its parent without setting HasChildEffect on the parent, so when the chain was effect → effectScope → effect, the outer's HasChildEffect bit was never set and run()'s pre-walk was skipped. The old inner's cleanup then fired after the new inner had already been set up. Mirror effect()'s flag-set inside effectScope(). Additionally, run()'s and updateComputed's pre-walks only unlinked deps matching `'fn' in dep`, so even with the flag set the loop skipped scope deps. Broaden the predicate to `!('getter' in dep) && !('currentValue' in dep)` to also match EffectScopeNode — unlinking a scope cascades through unwatched → effectScopeOper, which already disposes the scope's own deps in reverse. Bug 001 — computed unwatched FIFO: When a computed lost its last subscriber, the 'getter' in node branch of unwatched routed through purgeDeps, which walks deps from head to tail (FIFO). If the computed's getter had created multiple child effects, their cleanups ran in creation order instead of LIFO. Replace purgeDeps with a depsTail-backward walk in that branch. Added regression tests: - tests/effect.spec.ts: computed unwatched LIFO ordering. - tests/effectScope.spec.ts: scope-as-intermediate cleanup order. All 200 pass. --- src/index.ts | 16 ++++++++++++---- tests/effect.spec.ts | 16 ++++++++++++++++ tests/effectScope.spec.ts | 31 ++++++++++++++++++++++++++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/src/index.ts b/src/index.ts index f197700..aca2a5c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -76,9 +76,14 @@ const { } else if ('getter' in node) { if (node.depsTail !== undefined) { - node.depsTail = undefined; node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; - purgeDeps(node); + let link = node.depsTail; + node.depsTail = undefined; + while (link !== undefined) { + const prev = link.prevDep; + unlink(link, node); + link = prev; + } } } else if ('currentValue' in node) { @@ -200,6 +205,7 @@ export function effectScope(fn: () => void): () => void { const prevSub = setActiveSub(e); if (prevSub !== undefined) { link(e, prevSub, 0); + prevSub.flags |= HasChildEffect; } try { fn(); @@ -242,7 +248,8 @@ function updateComputed(c: ComputedNode): boolean { let link = c.depsTail; while (link !== undefined) { const prev = link.prevDep; - if ('fn' in link.dep) { + const dep = link.dep; + if (!('getter' in dep) && !('currentValue' in dep)) { unlink(link, c); } link = prev; @@ -280,7 +287,8 @@ function run(e: EffectNode): void { let link = e.depsTail; while (link !== undefined) { const prev = link.prevDep; - if ('fn' in link.dep) { + const dep = link.dep; + if (!('getter' in dep) && !('currentValue' in dep)) { unlink(link, e); } link = prev; diff --git a/tests/effect.spec.ts b/tests/effect.spec.ts index 3f6253f..98f6a01 100644 --- a/tests/effect.spec.ts +++ b/tests/effect.spec.ts @@ -129,6 +129,22 @@ test('three-level nested cleanup on dispose: deepest first (depth-first reverse) ]); }); +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 diff --git a/tests/effectScope.spec.ts b/tests/effectScope.spec.ts index 5bb0880..8746cf8 100644 --- a/tests/effectScope.spec.ts +++ b/tests/effectScope.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from 'vitest'; -import { effect, effectScope } from '../src'; +import { effect, effectScope, signal } from '../src'; test('scope dispose runs child effect cleanup', () => { const log: string[] = []; @@ -42,3 +42,32 @@ test('scope dispose: nested effect cleanup runs depth-first reverse', () => { 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', + ]); +}); From 24b134b5693e23a4fd2e73f095783cb835fc9dac Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Thu, 14 May 2026 22:09:41 +0800 Subject: [PATCH 3/5] fix: TS error in unwatched else-if branch The new reverse-walk inside the 'getter' in node branch confused tsslint's type inference because `node.depsTail` is narrowed to `Link` after the `!== undefined` check, but the loop variable needs to accept `Link | undefined` for the `link = prev` step. Import `Link` from system.js and annotate both `link` and `prev` explicitly. --- src/index.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index aca2a5c..5369fa3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import { createReactiveSystem, ReactiveFlags, type ReactiveNode } from './system.js'; +import { createReactiveSystem, ReactiveFlags, type Link, type ReactiveNode } from './system.js'; interface EffectScopeNode extends ReactiveNode { } @@ -77,10 +77,10 @@ const { else if ('getter' in node) { if (node.depsTail !== undefined) { node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; - let link = node.depsTail; + let link: Link | undefined = node.depsTail; node.depsTail = undefined; while (link !== undefined) { - const prev = link.prevDep; + const prev: Link | undefined = link.prevDep; unlink(link, node); link = prev; } From f772dac39d682feb12f976156502c53af36ece0e Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Thu, 14 May 2026 22:16:31 +0800 Subject: [PATCH 4/5] wip --- src/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 5369fa3..4dcadae 100644 --- a/src/index.ts +++ b/src/index.ts @@ -71,10 +71,7 @@ const { } }, unwatched(node: SignalNode | ComputedNode | EffectNode | EffectScopeNode) { - if ('fn' in node) { - effectOper.call(node); - } - else if ('getter' in node) { + if ('getter' in node) { if (node.depsTail !== undefined) { node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; let link: Link | undefined = node.depsTail; @@ -89,6 +86,9 @@ const { 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); } From a8bf2e1a1ac64524f59b29880c0b9aeac6c81cc5 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Thu, 14 May 2026 22:22:32 +0800 Subject: [PATCH 5/5] Update index.ts --- src/index.ts | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/index.ts b/src/index.ts index 4dcadae..800fe53 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import { createReactiveSystem, ReactiveFlags, type Link, type ReactiveNode } from './system.js'; +import { createReactiveSystem, ReactiveFlags, type ReactiveNode } from './system.js'; interface EffectScopeNode extends ReactiveNode { } @@ -74,13 +74,7 @@ const { if ('getter' in node) { if (node.depsTail !== undefined) { node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty; - let link: Link | undefined = node.depsTail; - node.depsTail = undefined; - while (link !== undefined) { - const prev: Link | undefined = link.prevDep; - unlink(link, node); - link = prev; - } + disposeAllDepsInReverse(node); } } else if ('currentValue' in node) { @@ -421,18 +415,22 @@ function effectOper(this: EffectNode): void { function effectScopeOper(this: EffectScopeNode): void { this.flags = ReactiveFlags.None; - let link = this.depsTail; - while (link !== undefined) { - const prev = link.prevDep; - unlink(link, this); - link = prev; - } + 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;