From 6c303eacd14b7b0de0accc228f6abeb43d706f63 Mon Sep 17 00:00:00 2001 From: Evan You Date: Fri, 28 Jun 2024 09:28:51 +0800 Subject: [PATCH 1/2] Revert "fix(reactivity): fix side effect computed dirty level (#11183)" This reverts commit 3bd79e3e5ed960fc42cbf77bc61a97d2c03557c0. --- .../reactivity/__tests__/computed.spec.ts | 57 ------------------- packages/reactivity/src/computed.ts | 6 +- packages/reactivity/src/effect.ts | 4 +- 3 files changed, 2 insertions(+), 65 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 9a91eed6389..0122f4e4391 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -708,63 +708,6 @@ describe('reactivity/computed', () => { expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) - it('should chained computeds keep reactivity when computed effect happens', async () => { - const v = ref('Hello') - const c = computed(() => { - v.value += ' World' - return v.value - }) - const d = computed(() => c.value) - const e = computed(() => d.value) - const Comp = { - setup: () => { - return () => d.value + ' | ' + e.value - }, - } - const root = nodeOps.createElement('div') - - render(h(Comp), root) - await nextTick() - expect(serializeInner(root)).toBe('Hello World | Hello World') - - v.value += ' World' - await nextTick() - expect(serializeInner(root)).toBe( - 'Hello World World World | Hello World World World', - ) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() - }) - - it('should keep dirty level when side effect computed value changed', () => { - const v = ref(0) - const c = computed(() => { - v.value += 1 - return v.value - }) - const d = computed(() => { - return { d: c.value } - }) - - const Comp = { - setup: () => { - return () => { - return [d.value.d, d.value.d] - } - }, - } - - const root = nodeOps.createElement('div') - render(h(Comp), root) - - expect(d.value.d).toBe(1) - expect(serializeInner(root)).toBe('11') - expect(c.effect._dirtyLevel).toBe( - DirtyLevels.MaybeDirty_ComputedSideEffect_Origin, - ) - expect(d.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty_ComputedSideEffect) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() - }) - it('debug: onTrigger (ref)', () => { let events: DebuggerEvent[] = [] const onTrigger = vi.fn((e: DebuggerEvent) => { diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index e764b60248c..91eac36012f 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -71,15 +71,11 @@ export class ComputedRefImpl { get value() { // the computed ref may get wrapped by other proxies e.g. readonly() #3376 const self = toRaw(this) - const lastDirtyLevel = self.effect._dirtyLevel if ( (!self._cacheable || self.effect.dirty) && hasChanged(self._value, (self._value = self.effect.run()!)) ) { - // keep dirty level when side effect computed's value changed - if (lastDirtyLevel !== DirtyLevels.MaybeDirty_ComputedSideEffect) { - triggerRefValue(self, DirtyLevels.Dirty) - } + triggerRefValue(self, DirtyLevels.Dirty) } trackRefValue(self) if ( diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 6817931f0e5..40868545b4b 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -93,10 +93,8 @@ export class ReactiveEffect { if ( dep.computed.effect._dirtyLevel === DirtyLevels.MaybeDirty_ComputedSideEffect_Origin - ) { - resetTracking() + ) return true - } triggerComputed(dep.computed) if (this._dirtyLevel >= DirtyLevels.Dirty) { break From e0df985f0317fb65c5b461bf224375c7763f0269 Mon Sep 17 00:00:00 2001 From: Evan You Date: Fri, 28 Jun 2024 09:31:14 +0800 Subject: [PATCH 2/2] 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 (