From 60f7a7b4c239e3c34856da5c2c4edb188c64795b Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 29 Sep 2024 19:35:55 +0800 Subject: [PATCH 1/5] fix(reactivity): prevent overwriting `next` property during batch processing --- packages/reactivity/__tests__/watch.spec.ts | 17 +++++++++++++++++ packages/reactivity/src/effect.ts | 3 +++ 2 files changed, 20 insertions(+) diff --git a/packages/reactivity/__tests__/watch.spec.ts b/packages/reactivity/__tests__/watch.spec.ts index 882e8a245fa..245acfd63be 100644 --- a/packages/reactivity/__tests__/watch.spec.ts +++ b/packages/reactivity/__tests__/watch.spec.ts @@ -260,4 +260,21 @@ describe('watch', () => { src.value = 10 expect(spy).toHaveBeenCalledTimes(2) }) + + test('should ensure correct execution order in batch processing', () => { + const dummy: number[] = [] + const n1 = ref(0) + const n2 = ref(0) + const sum = computed(() => n1.value + n2.value) + watch(n1, () => { + dummy.push(1) + n2.value++ + }) + watch(sum, () => dummy.push(2)) + watch(n1, () => dummy.push(3)) + + n1.value++ + + expect(dummy).toEqual([1, 2, 3]) + }) }) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index dad800d146d..49893d2683c 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -237,6 +237,9 @@ let batchedSub: Subscriber | undefined export function batch(sub: Subscriber): void { sub.flags |= EffectFlags.NOTIFIED + // If sub.next is set, the subscriber is already in a batch, + // return to avoid overwriting it and losing previous subscriptions. + if (sub.next) return sub.next = batchedSub batchedSub = sub } From 1503051f774d33b5a7eb4326ee830c6de494c682 Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 29 Sep 2024 20:03:39 +0800 Subject: [PATCH 2/5] fix: update --- packages/reactivity/src/effect.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 49893d2683c..1f985c6ea46 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -234,12 +234,20 @@ export class ReactiveEffect let batchDepth = 0 let batchedSub: Subscriber | undefined +const batchedCleanup: (() => void)[] = [] export function batch(sub: Subscriber): void { sub.flags |= EffectFlags.NOTIFIED // If sub.next is set, the subscriber is already in a batch, // return to avoid overwriting it and losing previous subscriptions. - if (sub.next) return + if (sub.next) { + batchedCleanup.push(() => { + if (!(sub.flags & EffectFlags.ACTIVE)) { + sub.flags &= ~EffectFlags.NOTIFIED + } + }) + return + } sub.next = batchedSub batchedSub = sub } @@ -273,6 +281,12 @@ export function endBatch(): void { } e = e.next } + if (batchedCleanup.length) { + for (let i = 0; i < batchedCleanup.length; i++) { + batchedCleanup[i]() + } + batchedCleanup.length = 0 + } e = batchedSub batchedSub = undefined // 2nd pass: run effects From ddad5729ae4cc623d5de42b0f86dec2f865edd73 Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 29 Sep 2024 22:20:55 +0800 Subject: [PATCH 3/5] refactor: move `NOTIFIED` cleanup for computed into `batchedCleanup` --- packages/reactivity/src/computed.ts | 2 +- packages/reactivity/src/effect.ts | 36 +++++++++++------------------ 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 628cf46367b..ea798e201d4 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -121,7 +121,7 @@ export class ComputedRefImpl implements Subscriber { // avoid infinite self recursion activeSub !== this ) { - batch(this) + batch(this, true) return true } else if (__DEV__) { // TODO warn diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 1f985c6ea46..3db4faf931e 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -236,12 +236,13 @@ let batchDepth = 0 let batchedSub: Subscriber | undefined const batchedCleanup: (() => void)[] = [] -export function batch(sub: Subscriber): void { +export function batch(sub: Subscriber, isComputed = false): void { sub.flags |= EffectFlags.NOTIFIED - // If sub.next is set, the subscriber is already in a batch, - // return to avoid overwriting it and losing previous subscriptions. - if (sub.next) { + if (isComputed) { batchedCleanup.push(() => { + // clear notified flags for computed upfront + // we use the ACTIVE flag as a discriminator between computed and effect, + // since NOTIFIED is useless for an inactive effect anyway. if (!(sub.flags & EffectFlags.ACTIVE)) { sub.flags &= ~EffectFlags.NOTIFIED } @@ -268,30 +269,19 @@ export function endBatch(): void { return } + if (batchedCleanup.length) { + for (let i = 0; i < batchedCleanup.length; i++) { + batchedCleanup[i]() + } + batchedCleanup.length = 0 + } + let error: unknown while (batchedSub) { let e: Subscriber | undefined = batchedSub - let next: Subscriber | undefined - // 1st pass: clear notified flags for computed upfront - // we use the ACTIVE flag as a discriminator between computed and effect, - // since NOTIFIED is useless for an inactive effect anyway. - while (e) { - if (!(e.flags & EffectFlags.ACTIVE)) { - e.flags &= ~EffectFlags.NOTIFIED - } - e = e.next - } - if (batchedCleanup.length) { - for (let i = 0; i < batchedCleanup.length; i++) { - batchedCleanup[i]() - } - batchedCleanup.length = 0 - } - e = batchedSub batchedSub = undefined - // 2nd pass: run effects while (e) { - next = e.next + const next: Subscriber | undefined = e.next e.next = undefined e.flags &= ~EffectFlags.NOTIFIED if (e.flags & EffectFlags.ACTIVE) { From 2468ba623a92e729ff575299eaf7a009a306d45d Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 29 Sep 2024 22:26:54 +0800 Subject: [PATCH 4/5] chore: tweaks --- packages/reactivity/src/effect.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 3db4faf931e..c6039b03830 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -240,12 +240,8 @@ export function batch(sub: Subscriber, isComputed = false): void { sub.flags |= EffectFlags.NOTIFIED if (isComputed) { batchedCleanup.push(() => { - // clear notified flags for computed upfront - // we use the ACTIVE flag as a discriminator between computed and effect, - // since NOTIFIED is useless for an inactive effect anyway. - if (!(sub.flags & EffectFlags.ACTIVE)) { - sub.flags &= ~EffectFlags.NOTIFIED - } + // clear notified flags for computed + sub.flags &= ~EffectFlags.NOTIFIED }) return } From 46c96ba38a14fa51f8ce453b0098b3b7f8a20d4a Mon Sep 17 00:00:00 2001 From: tycho Date: Mon, 30 Sep 2024 11:58:22 +0800 Subject: [PATCH 5/5] refactor: replaced array-based `batchedCleanup` with a linked list for computed subscribers --- packages/reactivity/src/effect.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index c6039b03830..243518839b3 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -234,15 +234,13 @@ export class ReactiveEffect let batchDepth = 0 let batchedSub: Subscriber | undefined -const batchedCleanup: (() => void)[] = [] +let batchedComputed: Subscriber | undefined export function batch(sub: Subscriber, isComputed = false): void { sub.flags |= EffectFlags.NOTIFIED if (isComputed) { - batchedCleanup.push(() => { - // clear notified flags for computed - sub.flags &= ~EffectFlags.NOTIFIED - }) + sub.next = batchedComputed + batchedComputed = sub return } sub.next = batchedSub @@ -265,11 +263,15 @@ export function endBatch(): void { return } - if (batchedCleanup.length) { - for (let i = 0; i < batchedCleanup.length; i++) { - batchedCleanup[i]() + if (batchedComputed) { + let e: Subscriber | undefined = batchedComputed + batchedComputed = undefined + while (e) { + const next: Subscriber | undefined = e.next + e.next = undefined + e.flags &= ~EffectFlags.NOTIFIED + e = next } - batchedCleanup.length = 0 } let error: unknown