fix(effect): re-fix should trigger inner effects in sequence

stackblitz · Oct 11, 2024 · 54436ab · 54436ab
1 parent 9e452c6
commit 54436ab
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 16 deletions.
diff --git a/lib/effect.ts b/lib/effect.ts
@@ -30,11 +30,19 @@ export class Effect implements IEffect, Dependency, Subscriber {
  }
 
  notify() {
- if (this.versionOrDirtyLevel === DirtyLevels.MaybeDirty) {
- Subscriber.resolveMaybeDirty(this);
- }
- if (this.versionOrDirtyLevel === DirtyLevels.Dirty) {
- this.run();
+ const dirtyLevel = this.versionOrDirtyLevel;
+ if (dirtyLevel === DirtyLevels.SideEffectsOnly) {
+ this.versionOrDirtyLevel = DirtyLevels.None;
+ Subscriber.runInnerEffects(this);
+ } else {
+ if (dirtyLevel === DirtyLevels.MaybeDirty) {
+ Subscriber.resolveMaybeDirty(this);
+ }
+ if (this.versionOrDirtyLevel === DirtyLevels.Dirty) {
+ this.run();
+ } else {
+ Subscriber.runInnerEffects(this);
+ }
  }
  }
 

diff --git a/lib/effectScope.ts b/lib/effectScope.ts
@@ -1,17 +1,26 @@
-import { DirtyLevels, Subscriber } from './system';
+import { DirtyLevels, IEffect, Subscriber } from './system';
 
 export function effectScope() {
  return new EffectScope();
 }
 
-export class EffectScope implements Subscriber {
+export class EffectScope implements IEffect, Subscriber {
+ nextNotify = undefined;
+
  // Subscriber
  deps = undefined;
  depsTail = undefined;
- versionOrDirtyLevel = DirtyLevels.Dirty;
+ versionOrDirtyLevel = DirtyLevels.None;
 
  notifyLostSubs() { }
 
+ notify() {
+ if (this.versionOrDirtyLevel !== DirtyLevels.None) {
+ this.versionOrDirtyLevel = DirtyLevels.None;
+ Subscriber.runInnerEffects(this);
+ }
+ }
+
  run<T>(fn: () => T) {
  const prevActiveSub = Subscriber.startTrack(this, true);
  const res = fn();

diff --git a/lib/system.ts b/lib/system.ts
@@ -33,7 +33,8 @@ export interface Link {
 }
 
 export const enum DirtyLevels {
- NotDirty,
+ None,
+ SideEffectsOnly,
  MaybeDirty,
  Dirty,
 }
@@ -228,6 +229,7 @@ export namespace Dependency {
  }
 
  export function propagate(dep: Dependency) {
+ let depIsEffect = false;
  let link = dep.subs;
  let dirtyLevel = DirtyLevels.Dirty;
  let depth = 0;
@@ -242,19 +244,25 @@ export namespace Dependency {
  sub.versionOrDirtyLevel = dirtyLevel;
  }
 
- if (subDirtyLevel === DirtyLevels.NotDirty) {
+ if (subDirtyLevel === DirtyLevels.None) {
+ const subIsEffect = 'notify' in sub;
 
  if ('subs' in sub) {
  sub.deps!.prevPropagateOrNextReleased = link;
  dep = sub;
+ depIsEffect = subIsEffect;
  link = sub.subs;
- dirtyLevel = DirtyLevels.MaybeDirty;
+ if (subIsEffect) {
+ dirtyLevel = DirtyLevels.SideEffectsOnly;
+ } else {
+ dirtyLevel = DirtyLevels.MaybeDirty;
+ }
  depth++;
 
  continue top;
  }
 
- if ('notify' in sub) {
+ if (subIsEffect) {
  const queuedEffectsTail = system.queuedEffectsTail;
 
  if (queuedEffectsTail !== undefined) {
@@ -278,11 +286,16 @@ export namespace Dependency {
  if (prevLink !== undefined) {
  depDeps.prevPropagateOrNextReleased = undefined;
  dep = prevLink.dep;
+ depIsEffect = 'notify' in dep;
  link = prevLink.nextSub;
  depth--;
 
  if (depth === 0) {
  dirtyLevel = DirtyLevels.Dirty;
+ } else if (depIsEffect) {
+ dirtyLevel = DirtyLevels.SideEffectsOnly;
+ } else {
+ dirtyLevel = DirtyLevels.MaybeDirty;
  }
 
  const prevSub = prevLink.sub;
@@ -312,6 +325,17 @@ export namespace Subscriber {
 
  const system = System;
 
+ export function runInnerEffects(sub: Subscriber) {
+ let link = sub.deps as Link | undefined;
+ while (link !== undefined) {
+ const dep = link.dep as Dependency | Dependency & IEffect;
+ if ('notify' in dep) {
+ dep.notify();
+ }
+ link = link.nextDep;
+ }
+ }
+
  export function resolveMaybeDirty(sub: Subscriber) {
  let link = sub.deps;
 
@@ -344,7 +368,7 @@ export namespace Subscriber {
  const dirtyLevel = sub.versionOrDirtyLevel;
 
  if (dirtyLevel === DirtyLevels.MaybeDirty) {
- sub.versionOrDirtyLevel = DirtyLevels.NotDirty;
+ sub.versionOrDirtyLevel = DirtyLevels.None;
  }
 
  const subSubs = (sub as Dependency & Subscriber).subs;
@@ -417,6 +441,6 @@ export namespace Subscriber {
  Link.release(sub.deps);
  sub.deps = undefined;
  }
- sub.versionOrDirtyLevel = DirtyLevels.NotDirty;
+ sub.versionOrDirtyLevel = DirtyLevels.None;
  }
 }
diff --git a/tests/effect.spec.ts b/tests/effect.spec.ts
@@ -89,12 +89,14 @@ test('should not trigger inner effect when resolve maybe dirty', () => {
  a.set(2);
 });
 
-test.skip('should trigger inner effects in sequence', () => {
+test('should trigger inner effects in sequence', () => {
  const a = signal(0);
  const b = signal(0);
+ const c = computed(() => a.get() - b.get());
  const order: string[] = [];
 
  effect(() => {
+ c.get();
 
  effect(() => {
  order.push('first inner');
@@ -118,7 +120,7 @@ test.skip('should trigger inner effects in sequence', () => {
  expect(order).toEqual(['first inner', 'last inner']);
 });
 
-test.skip('should trigger inner effects in sequence in effect scope', () => {
+test('should trigger inner effects in sequence in effect scope', () => {
  const a = signal(0);
  const b = signal(0);
  const scope = effectScope();