From 68a7c5fcbfea614c997edfb501e6d76f7c32388e Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 09:05:08 +0800 Subject: [PATCH 1/7] fix(reactivity): ensure watch cleanup on effect stop --- packages/reactivity/src/effectScope.ts | 5 +++-- packages/reactivity/src/watch.ts | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/reactivity/src/effectScope.ts b/packages/reactivity/src/effectScope.ts index 98e45fb5707..46e7139d655 100644 --- a/packages/reactivity/src/effectScope.ts +++ b/packages/reactivity/src/effectScope.ts @@ -122,8 +122,9 @@ export class EffectScope { for (i = 0, l = this.effects.length; i < l; i++) { this.effects[i].stop() } - for (i = 0, l = this.cleanups.length; i < l; i++) { - this.cleanups[i]() + const cleanups = this.cleanups.slice() + for (i = 0, l = cleanups.length; i < l; i++) { + cleanups[i]() } if (this.scopes) { for (i = 0, l = this.scopes.length; i < l; i++) { diff --git a/packages/reactivity/src/watch.ts b/packages/reactivity/src/watch.ts index 659121ca34b..0e47c84ccd7 100644 --- a/packages/reactivity/src/watch.ts +++ b/packages/reactivity/src/watch.ts @@ -215,8 +215,12 @@ export function watch( effect.stop() if (scope && scope.active) { remove(scope.effects, effect) + remove(scope.cleanups, watchHandle) } } + if (scope) { + scope.cleanups.push(watchHandle) + } if (once && cb) { const _cb = cb From c090c5ed3a7b9a377726a7b581df5aad32831c0b Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 09:14:04 +0800 Subject: [PATCH 2/7] test: add test --- packages/reactivity/__tests__/watch.spec.ts | 42 +++++++++++++++++++++ packages/reactivity/src/effectScope.ts | 5 ++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/reactivity/__tests__/watch.spec.ts b/packages/reactivity/__tests__/watch.spec.ts index 245acfd63be..680de91f1d9 100644 --- a/packages/reactivity/__tests__/watch.spec.ts +++ b/packages/reactivity/__tests__/watch.spec.ts @@ -5,10 +5,12 @@ import { type WatchOptions, type WatchScheduler, computed, + effectScope, onWatcherCleanup, ref, watch, } from '../src' +import { expect } from 'vitest' const queue: (() => void)[] = [] @@ -277,4 +279,44 @@ describe('watch', () => { expect(dummy).toEqual([1, 2, 3]) }) + + test('removing a watcher while stopping its effectScope', async () => { + const count = ref(0) + const scope = effectScope() + let watcherCalls = 0 + let cleanupCalls = 0 + + scope.run(() => { + const stop1 = watch(count, () => { + watcherCalls++ + }) + watch(count, (val, old, onCleanup) => { + watcherCalls++ + onCleanup(() => { + cleanupCalls++ + stop1() + }) + }) + watch(count, () => { + watcherCalls++ + }) + }) + + expect(watcherCalls).toBe(0) + expect(cleanupCalls).toBe(0) + + count.value++ + await nextTick() + expect(watcherCalls).toBe(3) + expect(cleanupCalls).toBe(0) + + scope.stop() + count.value++ + await nextTick() + expect(watcherCalls).toBe(3) + expect(cleanupCalls).toBe(1) + + expect(scope.effects.length).toBe(0) + expect(scope.cleanups.length).toBe(0) + }) }) diff --git a/packages/reactivity/src/effectScope.ts b/packages/reactivity/src/effectScope.ts index 46e7139d655..b18e7cd7b6f 100644 --- a/packages/reactivity/src/effectScope.ts +++ b/packages/reactivity/src/effectScope.ts @@ -119,8 +119,9 @@ export class EffectScope { if (this._active) { this._active = false let i, l - for (i = 0, l = this.effects.length; i < l; i++) { - this.effects[i].stop() + const effects = this.effects.slice() + for (i = 0, l = effects.length; i < l; i++) { + effects[i].stop() } const cleanups = this.cleanups.slice() for (i = 0, l = cleanups.length; i < l; i++) { From 0c97724203c99ef7cd1ea0ce5b9dea70635749ca Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 09:15:12 +0800 Subject: [PATCH 3/7] test: add test --- packages/reactivity/__tests__/watch.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/reactivity/__tests__/watch.spec.ts b/packages/reactivity/__tests__/watch.spec.ts index 680de91f1d9..a5a55f8eb37 100644 --- a/packages/reactivity/__tests__/watch.spec.ts +++ b/packages/reactivity/__tests__/watch.spec.ts @@ -10,7 +10,6 @@ import { ref, watch, } from '../src' -import { expect } from 'vitest' const queue: (() => void)[] = [] From 1cb6981b896e897957b592247ba03f7e91c6d4b4 Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 14:39:28 +0800 Subject: [PATCH 4/7] chore: update --- packages/reactivity/__tests__/watch.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/reactivity/__tests__/watch.spec.ts b/packages/reactivity/__tests__/watch.spec.ts index a5a55f8eb37..7f4a8830a62 100644 --- a/packages/reactivity/__tests__/watch.spec.ts +++ b/packages/reactivity/__tests__/watch.spec.ts @@ -279,7 +279,7 @@ describe('watch', () => { expect(dummy).toEqual([1, 2, 3]) }) - test('removing a watcher while stopping its effectScope', async () => { + test('clean up watchers during effect scope stop', async () => { const count = ref(0) const scope = effectScope() let watcherCalls = 0 From d05ea4abf217abb05bbbfe795de8920f3340770a Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 14:53:26 +0800 Subject: [PATCH 5/7] chore: update --- packages/reactivity/src/watch.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/reactivity/src/watch.ts b/packages/reactivity/src/watch.ts index 0e47c84ccd7..28f141ea1a0 100644 --- a/packages/reactivity/src/watch.ts +++ b/packages/reactivity/src/watch.ts @@ -23,7 +23,7 @@ import { } from './effect' import { isReactive, isShallow } from './reactive' import { type Ref, isRef } from './ref' -import { getCurrentScope } from './effectScope' +import { getCurrentScope, onScopeDispose } from './effectScope' // These errors were transferred from `packages/runtime-core/src/errorHandling.ts` // to @vue/reactivity to allow co-location with the moved base watch logic, hence @@ -218,9 +218,7 @@ export function watch( remove(scope.cleanups, watchHandle) } } - if (scope) { - scope.cleanups.push(watchHandle) - } + onScopeDispose(watchHandle, true) if (once && cb) { const _cb = cb From d2fd005174415d6cff0f737986936433ed15df5c Mon Sep 17 00:00:00 2001 From: daiwei Date: Wed, 13 Nov 2024 14:55:38 +0800 Subject: [PATCH 6/7] chore: update test --- .../reactivity/__tests__/effectScope.spec.ts | 3 ++ packages/reactivity/__tests__/watch.spec.ts | 41 ------------------- 2 files changed, 3 insertions(+), 41 deletions(-) diff --git a/packages/reactivity/__tests__/effectScope.spec.ts b/packages/reactivity/__tests__/effectScope.spec.ts index 537cac64c6b..22c00cc58cf 100644 --- a/packages/reactivity/__tests__/effectScope.spec.ts +++ b/packages/reactivity/__tests__/effectScope.spec.ts @@ -358,5 +358,8 @@ describe('reactivity/effect/scope', () => { await nextTick() expect(watcherCalls).toBe(3) expect(cleanupCalls).toBe(1) + + expect(scope.effects.length).toBe(0) + expect(scope.cleanups.length).toBe(0) }) }) diff --git a/packages/reactivity/__tests__/watch.spec.ts b/packages/reactivity/__tests__/watch.spec.ts index 7f4a8830a62..245acfd63be 100644 --- a/packages/reactivity/__tests__/watch.spec.ts +++ b/packages/reactivity/__tests__/watch.spec.ts @@ -5,7 +5,6 @@ import { type WatchOptions, type WatchScheduler, computed, - effectScope, onWatcherCleanup, ref, watch, @@ -278,44 +277,4 @@ describe('watch', () => { expect(dummy).toEqual([1, 2, 3]) }) - - test('clean up watchers during effect scope stop', async () => { - const count = ref(0) - const scope = effectScope() - let watcherCalls = 0 - let cleanupCalls = 0 - - scope.run(() => { - const stop1 = watch(count, () => { - watcherCalls++ - }) - watch(count, (val, old, onCleanup) => { - watcherCalls++ - onCleanup(() => { - cleanupCalls++ - stop1() - }) - }) - watch(count, () => { - watcherCalls++ - }) - }) - - expect(watcherCalls).toBe(0) - expect(cleanupCalls).toBe(0) - - count.value++ - await nextTick() - expect(watcherCalls).toBe(3) - expect(cleanupCalls).toBe(0) - - scope.stop() - count.value++ - await nextTick() - expect(watcherCalls).toBe(3) - expect(cleanupCalls).toBe(1) - - expect(scope.effects.length).toBe(0) - expect(scope.cleanups.length).toBe(0) - }) }) From 0073a018a060f3bef6c58c79400bfaead6381f90 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 14 Nov 2024 08:44:28 +0800 Subject: [PATCH 7/7] chore: update test --- packages/reactivity/__tests__/effectScope.spec.ts | 2 +- packages/reactivity/src/effectScope.ts | 11 ++++++++--- packages/reactivity/src/watch.ts | 4 +--- packages/runtime-core/__tests__/apiWatch.spec.ts | 3 +-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/reactivity/__tests__/effectScope.spec.ts b/packages/reactivity/__tests__/effectScope.spec.ts index 22c00cc58cf..debbdafb1e7 100644 --- a/packages/reactivity/__tests__/effectScope.spec.ts +++ b/packages/reactivity/__tests__/effectScope.spec.ts @@ -176,7 +176,7 @@ describe('reactivity/effect/scope', () => { expect('[Vue warn] cannot run an inactive effect scope.').toHaveBeenWarned() - expect(scope.effects.length).toBe(1) + expect(scope.effects.length).toBe(0) counter.num = 7 expect(dummy).toBe(0) diff --git a/packages/reactivity/src/effectScope.ts b/packages/reactivity/src/effectScope.ts index b18e7cd7b6f..e045d30e89b 100644 --- a/packages/reactivity/src/effectScope.ts +++ b/packages/reactivity/src/effectScope.ts @@ -123,15 +123,20 @@ export class EffectScope { for (i = 0, l = effects.length; i < l; i++) { effects[i].stop() } - const cleanups = this.cleanups.slice() - for (i = 0, l = cleanups.length; i < l; i++) { - cleanups[i]() + this.effects.length = 0 + + for (i = 0, l = this.cleanups.length; i < l; i++) { + this.cleanups[i]() } + this.cleanups.length = 0 + if (this.scopes) { for (i = 0, l = this.scopes.length; i < l; i++) { this.scopes[i].stop(true) } + this.scopes.length = 0 } + // nested scope, dereference from parent to avoid memory leaks if (!this.detached && this.parent && !fromParent) { // optimized O(1) removal diff --git a/packages/reactivity/src/watch.ts b/packages/reactivity/src/watch.ts index 28f141ea1a0..659121ca34b 100644 --- a/packages/reactivity/src/watch.ts +++ b/packages/reactivity/src/watch.ts @@ -23,7 +23,7 @@ import { } from './effect' import { isReactive, isShallow } from './reactive' import { type Ref, isRef } from './ref' -import { getCurrentScope, onScopeDispose } from './effectScope' +import { getCurrentScope } from './effectScope' // These errors were transferred from `packages/runtime-core/src/errorHandling.ts` // to @vue/reactivity to allow co-location with the moved base watch logic, hence @@ -215,10 +215,8 @@ export function watch( effect.stop() if (scope && scope.active) { remove(scope.effects, effect) - remove(scope.cleanups, watchHandle) } } - onScopeDispose(watchHandle, true) if (once && cb) { const _cb = cb diff --git a/packages/runtime-core/__tests__/apiWatch.spec.ts b/packages/runtime-core/__tests__/apiWatch.spec.ts index 10a4fe659e0..7d2a1e73c08 100644 --- a/packages/runtime-core/__tests__/apiWatch.spec.ts +++ b/packages/runtime-core/__tests__/apiWatch.spec.ts @@ -25,7 +25,6 @@ import { } from '@vue/runtime-test' import { type DebuggerEvent, - EffectFlags, ITERATE_KEY, type Ref, type ShallowRef, @@ -1341,7 +1340,7 @@ describe('api: watch', () => { await nextTick() await nextTick() - expect(instance!.scope.effects[0].flags & EffectFlags.ACTIVE).toBeFalsy() + expect(instance!.scope.effects.length).toBe(0) }) test('this.$watch should pass `this.proxy` to watch source as the first argument ', () => {