From e0df985f0317fb65c5b461bf224375c7763f0269 Mon Sep 17 00:00:00 2001 From: Evan You Date: Fri, 28 Jun 2024 09:31:14 +0800 Subject: [PATCH] fix: Revert "fix(reactivity): avoid infinite loop when render access a side effect computed (#11135)" This reverts commit 8296e19855e369a7826f5ea26540a6da01dc7093. --- .../reactivity/__tests__/computed.spec.ts | 101 ++---------------- packages/reactivity/src/computed.ts | 5 +- packages/reactivity/src/constants.ts | 11 +- packages/reactivity/src/effect.ts | 29 ----- 4 files changed, 12 insertions(+), 134 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 0122f4e4391..10c09109fdb 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -456,78 +456,6 @@ describe('reactivity/computed', () => { expect(fnSpy).toBeCalledTimes(2) }) - it('should mark dirty as MaybeDirty_ComputedSideEffect_Origin', () => { - const v = ref(1) - const c = computed(() => { - v.value += 1 - return v.value - }) - - c.value - expect(c.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() - }) - - it('should not infinite re-run effect when effect access original side effect computed', async () => { - const spy = vi.fn() - const v = ref(0) - const c = computed(() => { - v.value += 1 - return v.value - }) - const Comp = { - setup: () => { - return () => { - spy() - return v.value + c.value - } - }, - } - const root = nodeOps.createElement('div') - - render(h(Comp), root) - expect(spy).toBeCalledTimes(1) - await nextTick() - expect(c.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(serializeInner(root)).toBe('2') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() - }) - - it('should not infinite re-run effect when effect access chained side effect computed', async () => { - const spy = vi.fn() - const v = ref(0) - const c1 = computed(() => { - v.value += 1 - return v.value - }) - const c2 = computed(() => v.value + c1.value) - const Comp = { - setup: () => { - return () => { - spy() - return v.value + c1.value + c2.value - } - }, - } - const root = nodeOps.createElement('div') - - render(h(Comp), root) - expect(spy).toBeCalledTimes(1) - await nextTick() - expect(c1.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(c2.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect, - ) - expect(serializeInner(root)).toBe('4') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() - }) - it('should chained recurse effects clear dirty after trigger', () => { const v = ref(1) const c1 = computed(() => v.value) @@ -554,9 +482,7 @@ describe('reactivity/computed', () => { c3.value - expect(c1.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) + expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.effect._dirtyLevel).toBe( DirtyLevels.MaybeDirty_ComputedSideEffect, ) @@ -576,9 +502,7 @@ describe('reactivity/computed', () => { }) const c2 = computed(() => v.value + c1.value) expect(c2.value).toBe('0foo') - expect(c2.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect, - ) + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.value).toBe('1foo') expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) @@ -599,12 +523,8 @@ describe('reactivity/computed', () => { c2.value }) expect(fnSpy).toBeCalledTimes(1) - expect(c1.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(c2.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect, - ) + expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty) + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) v.value = 2 expect(fnSpy).toBeCalledTimes(2) expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() @@ -637,9 +557,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) c3.value - expect(c1.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) + expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.effect._dirtyLevel).toBe( DirtyLevels.MaybeDirty_ComputedSideEffect, ) @@ -693,18 +611,11 @@ describe('reactivity/computed', () => { render(h(Comp), root) await nextTick() - expect(c.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) expect(serializeInner(root)).toBe('Hello World') v.value += ' World' - expect(c.effect._dirtyLevel).toBe(DirtyLevels.Dirty) await nextTick() - expect(c.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(serializeInner(root)).toBe('Hello World World World') + expect(serializeInner(root)).toBe('Hello World World World World') expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 91eac36012f..da63fe84750 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -78,10 +78,7 @@ export class ComputedRefImpl { triggerRefValue(self, DirtyLevels.Dirty) } trackRefValue(self) - if ( - self.effect._dirtyLevel >= - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin - ) { + if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty_ComputedSideEffect) { if (__DEV__ && (__TEST__ || this._warnRecursive)) { warn(COMPUTED_SIDE_EFFECT_WARN, `\n\ngetter: `, this.getter) } diff --git a/packages/reactivity/src/constants.ts b/packages/reactivity/src/constants.ts index 4898d691703..baa75d61644 100644 --- a/packages/reactivity/src/constants.ts +++ b/packages/reactivity/src/constants.ts @@ -23,10 +23,9 @@ export enum ReactiveFlags { } export enum DirtyLevels { - NotDirty, - QueryingDirty, - MaybeDirty_ComputedSideEffect_Origin, - MaybeDirty_ComputedSideEffect, - MaybeDirty, - Dirty, + NotDirty = 0, + QueryingDirty = 1, + MaybeDirty_ComputedSideEffect = 2, + MaybeDirty = 3, + Dirty = 4, } diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 40868545b4b..1528f4b1d89 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -76,9 +76,6 @@ export class ReactiveEffect { } public get dirty() { - // treat original side effect computed as not dirty to avoid infinite loop - if (this._dirtyLevel === DirtyLevels.MaybeDirty_ComputedSideEffect_Origin) - return false if ( this._dirtyLevel === DirtyLevels.MaybeDirty_ComputedSideEffect || this._dirtyLevel === DirtyLevels.MaybeDirty @@ -88,13 +85,6 @@ export class ReactiveEffect { for (let i = 0; i < this._depsLength; i++) { const dep = this.deps[i] if (dep.computed) { - // treat chained side effect computed as dirty to force it re-run - // since we know the original side effect computed is dirty - if ( - dep.computed.effect._dirtyLevel === - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin - ) - return true triggerComputed(dep.computed) if (this._dirtyLevel >= DirtyLevels.Dirty) { break @@ -308,30 +298,11 @@ export function triggerEffects( for (const effect of dep.keys()) { // dep.get(effect) is very expensive, we need to calculate it lazily and reuse the result let tracking: boolean | undefined - - if (!dep.computed && effect.computed) { - if ( - effect._runnings > 0 && - (tracking ??= dep.get(effect) === effect._trackId) - ) { - effect._dirtyLevel = DirtyLevels.MaybeDirty_ComputedSideEffect_Origin - continue - } - } - if ( effect._dirtyLevel < dirtyLevel && (tracking ??= dep.get(effect) === effect._trackId) ) { effect._shouldSchedule ||= effect._dirtyLevel === DirtyLevels.NotDirty - // always schedule if the computed is original side effect - // since we know it is actually dirty - if ( - effect.computed && - effect._dirtyLevel === DirtyLevels.MaybeDirty_ComputedSideEffect_Origin - ) { - effect._shouldSchedule = true - } effect._dirtyLevel = dirtyLevel } if (