Skip to content

Commit 18dcb4b

Browse files
johnsoncodehkjohnsoncodehk
committed
fix: EffectScope cleanup also runs child effects in reverse order
Previously the dispose helpers split into two camps: - EffectNode (effectOper) walked deps in reverse via the new disposeChildEffectsInReverse helper (LIFO) before the node's own cleanup ran. - EffectScopeNode (effectScopeOper) and the unwatched else-if branch for computeds still routed through purgeDeps, which walks deps from head to tail. For an EffectScope with multiple sibling effects, that meant cleanup ran in creation order (FIFO) instead of the reverse-creation (LIFO) order semantically required. Add disposeAllDepsInReverse and use it from effectScopeOper and the unwatched else-if. Same iteration shape as disposeChildEffectsInReverse but unlinks every dep — appropriate for full teardown where there's nothing to keep around. (purgeDeps still has its own niche: cleaning up unmarked deps after effect or computed re-evaluation, where reverse order would be wrong because depsTail must stay fixed.) Tests: new effectScope.spec.ts covers single-cleanup, sibling LIFO, and depth-first reverse for nested cases. All 196 pass.
1 parent fbabdbd commit 18dcb4b

3 files changed

Lines changed: 188 additions & 5 deletions

File tree

‎src/index.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,15 @@ const {
6666
},
6767
unwatched(node) {
6868
if (!(node.flags & ReactiveFlags.Mutable)) {
69-
effectScopeOper.call(node);
69+
if ('fn' in node) {
70+
effectOper.call(node as EffectNode);
71+
}
72+
else {
73+
effectScopeOper.call(node as EffectScopeNode);
74+
}
7075
} else if (node.depsTail !== undefined) {
71-
node.depsTail = undefined;
7276
node.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty;
73-
purgeDeps(node);
77+
disposeAllDepsInReverse(node);
7478
}
7579
},
7680
});
@@ -250,6 +254,7 @@ function run(e: EffectNode): void {
250254
&& checkDirty(e.deps!, e)
251255
)
252256
) {
257+
disposeChildEffectsInReverse(e);
253258
if (e.cleanup) {
254259
runCleanup(e);
255260
if (!e.flags) {
@@ -369,22 +374,43 @@ function runCleanup(e: EffectNode): void {
369374
}
370375

371376
function effectOper(this: EffectNode): void {
377+
disposeChildEffectsInReverse(this);
372378
if (this.cleanup) {
373379
runCleanup(this);
374380
}
375381
effectScopeOper.call(this);
376382
}
377383

384+
function disposeChildEffectsInReverse(parent: ReactiveNode): void {
385+
let link = parent.depsTail;
386+
while (link !== undefined) {
387+
const prev = link.prevDep;
388+
if ('fn' in link.dep) {
389+
unlink(link, parent);
390+
}
391+
link = prev;
392+
}
393+
}
394+
378395
function effectScopeOper(this: EffectScopeNode): void {
379-
this.depsTail = undefined;
380396
this.flags = ReactiveFlags.None;
381-
purgeDeps(this);
397+
disposeAllDepsInReverse(this);
382398
const sub = this.subs;
383399
if (sub !== undefined) {
384400
unlink(sub);
385401
}
386402
}
387403

404+
function disposeAllDepsInReverse(sub: ReactiveNode): void {
405+
let link = sub.depsTail;
406+
sub.depsTail = undefined;
407+
while (link !== undefined) {
408+
const prev = link.prevDep;
409+
unlink(link, sub);
410+
link = prev;
411+
}
412+
}
413+
388414
function purgeDeps(sub: ReactiveNode) {
389415
const depsTail = sub.depsTail;
390416
let dep = depsTail !== undefined ? depsTail.nextDep : sub.deps;

‎tests/effect.spec.ts‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,119 @@ test('should support custom recurse effect', () => {
1616
expect(triggers).toBe(6);
1717
});
1818

19+
test('cleanup order on outer re-run: inner before outer, before new run', () => {
20+
const log: string[] = [];
21+
const a = signal(0);
22+
23+
effect(() => {
24+
a();
25+
log.push('outer:run');
26+
effect(() => {
27+
log.push('inner:run');
28+
return () => log.push('inner:cleanup');
29+
});
30+
return () => log.push('outer:cleanup');
31+
});
32+
expect(log).toEqual(['outer:run', 'inner:run']);
33+
34+
log.length = 0;
35+
a(1);
36+
expect(log).toEqual([
37+
'inner:cleanup',
38+
'outer:cleanup',
39+
'outer:run',
40+
'inner:run',
41+
]);
42+
});
43+
44+
test('cleanup order on dispose: inner before outer', () => {
45+
const log: string[] = [];
46+
47+
const dispose = effect(() => {
48+
log.push('outer:run');
49+
effect(() => {
50+
log.push('inner:run');
51+
return () => log.push('inner:cleanup');
52+
});
53+
return () => log.push('outer:cleanup');
54+
});
55+
log.length = 0;
56+
dispose();
57+
expect(log).toEqual(['inner:cleanup', 'outer:cleanup']);
58+
});
59+
60+
test('sibling cleanup order on dispose: reverse creation (LIFO)', () => {
61+
const log: string[] = [];
62+
63+
const dispose = effect(() => {
64+
effect(() => {
65+
return () => log.push('inner1:cleanup');
66+
});
67+
effect(() => {
68+
return () => log.push('inner2:cleanup');
69+
});
70+
effect(() => {
71+
return () => log.push('inner3:cleanup');
72+
});
73+
return () => log.push('outer:cleanup');
74+
});
75+
dispose();
76+
expect(log).toEqual([
77+
'inner3:cleanup',
78+
'inner2:cleanup',
79+
'inner1:cleanup',
80+
'outer:cleanup',
81+
]);
82+
});
83+
84+
test('sibling cleanup order on outer re-run: reverse creation (LIFO)', () => {
85+
const log: string[] = [];
86+
const a = signal(0);
87+
88+
effect(() => {
89+
a();
90+
effect(() => {
91+
return () => log.push('inner1:cleanup');
92+
});
93+
effect(() => {
94+
return () => log.push('inner2:cleanup');
95+
});
96+
effect(() => {
97+
return () => log.push('inner3:cleanup');
98+
});
99+
return () => log.push('outer:cleanup');
100+
});
101+
log.length = 0;
102+
103+
a(1);
104+
expect(log.slice(0, 4)).toEqual([
105+
'inner3:cleanup',
106+
'inner2:cleanup',
107+
'inner1:cleanup',
108+
'outer:cleanup',
109+
]);
110+
});
111+
112+
test('three-level nested cleanup on dispose: deepest first (depth-first reverse)', () => {
113+
const log: string[] = [];
114+
115+
const dispose = effect(() => {
116+
effect(() => {
117+
effect(() => {
118+
return () => log.push('grandchild:cleanup');
119+
});
120+
return () => log.push('child:cleanup');
121+
});
122+
return () => log.push('outer:cleanup');
123+
});
124+
dispose();
125+
expect(log).toEqual([
126+
'grandchild:cleanup',
127+
'child:cleanup',
128+
'outer:cleanup',
129+
]);
130+
});
131+
19132
// https://github.com/stackblitz/alien-signals/issues/115
20133
test('outer effect keeps responding to its own dep after inner re-runs', () => {
21134
const a = signal(0);

‎tests/effectScope.spec.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { expect, test } from 'vitest';
2+
import { effect, effectScope } from '../src';
3+
4+
test('scope dispose runs child effect cleanup', () => {
5+
const log: string[] = [];
6+
const dispose = effectScope(() => {
7+
effect(() => {
8+
return () => log.push('inner:cleanup');
9+
});
10+
});
11+
dispose();
12+
expect(log).toEqual(['inner:cleanup']);
13+
});
14+
15+
test('scope dispose: sibling effects clean up in reverse creation (LIFO)', () => {
16+
const log: string[] = [];
17+
const dispose = effectScope(() => {
18+
effect(() => {
19+
return () => log.push('e1:cleanup');
20+
});
21+
effect(() => {
22+
return () => log.push('e2:cleanup');
23+
});
24+
effect(() => {
25+
return () => log.push('e3:cleanup');
26+
});
27+
});
28+
dispose();
29+
expect(log).toEqual(['e3:cleanup', 'e2:cleanup', 'e1:cleanup']);
30+
});
31+
32+
test('scope dispose: nested effect cleanup runs depth-first reverse', () => {
33+
const log: string[] = [];
34+
const dispose = effectScope(() => {
35+
effect(() => {
36+
effect(() => {
37+
return () => log.push('grandchild:cleanup');
38+
});
39+
return () => log.push('child:cleanup');
40+
});
41+
});
42+
dispose();
43+
expect(log).toEqual(['grandchild:cleanup', 'child:cleanup']);
44+
});

0 commit comments

Comments
 (0)