From 6c7e0bd88f021b0b6365370e97b0c7e243d7d70b Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 6 Feb 2024 18:23:56 +0800 Subject: [PATCH] fix(reactivity): handle `MaybeDirty` recurse (#10187) close #10185 --- .../reactivity/__tests__/computed.spec.ts | 40 ++++++++++++++++++ packages/reactivity/src/computed.ts | 12 +++--- packages/reactivity/src/constants.ts | 5 ++- packages/reactivity/src/effect.ts | 42 +++++++++---------- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 860e4dab154..5a0beb973e8 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -10,6 +10,7 @@ import { isReadonly, reactive, ref, + shallowRef, toRaw, } from '../src' import { DirtyLevels } from '../src/constants' @@ -521,6 +522,45 @@ describe('reactivity/computed', () => { expect(fnSpy).toBeCalledTimes(2) }) + // #10185 + it('should not override queried MaybeDirty result', () => { + class Item { + v = ref(0) + } + const v1 = shallowRef() + const v2 = ref(false) + const c1 = computed(() => { + let c = v1.value + if (!v1.value) { + c = new Item() + v1.value = c + } + return c.v.value + }) + const c2 = computed(() => { + if (!v2.value) return 'no' + return c1.value ? 'yes' : 'no' + }) + const c3 = computed(() => c2.value) + + c3.value + v2.value = true + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) + expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + + c3.value + expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty) + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + + v1.value.v.value = 999 + expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty) + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + + expect(c3.value).toBe('yes') + }) + it('should be not dirty after deps mutate (mutate deps in computed)', async () => { const state = reactive({}) const consumer = computed(() => { diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 03459c7dfb6..9eed5bc8399 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -1,4 +1,4 @@ -import { type DebuggerOptions, ReactiveEffect, scheduleEffects } from './effect' +import { type DebuggerOptions, ReactiveEffect } from './effect' import { type Ref, trackRefValue, triggerRefValue } from './ref' import { NOOP, hasChanged, isFunction } from '@vue/shared' import { toRaw } from './reactive' @@ -44,7 +44,6 @@ export class ComputedRefImpl { this.effect = new ReactiveEffect( () => getter(this._value), () => triggerRefValue(this, DirtyLevels.MaybeDirty), - () => this.dep && scheduleEffects(this.dep), ) this.effect.computed = this this.effect.active = this._cacheable = !isSSR @@ -54,10 +53,11 @@ export class ComputedRefImpl { get value() { // the computed ref may get wrapped by other proxies e.g. readonly() #3376 const self = toRaw(this) - if (!self._cacheable || self.effect.dirty) { - if (hasChanged(self._value, (self._value = self.effect.run()!))) { - triggerRefValue(self, DirtyLevels.Dirty) - } + if ( + (!self._cacheable || self.effect.dirty) && + hasChanged(self._value, (self._value = self.effect.run()!)) + ) { + triggerRefValue(self, DirtyLevels.Dirty) } trackRefValue(self) if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty) { diff --git a/packages/reactivity/src/constants.ts b/packages/reactivity/src/constants.ts index 1e3483eb30d..5e9716dd88e 100644 --- a/packages/reactivity/src/constants.ts +++ b/packages/reactivity/src/constants.ts @@ -24,6 +24,7 @@ export enum ReactiveFlags { export enum DirtyLevels { NotDirty = 0, - MaybeDirty = 1, - Dirty = 2, + QueryingDirty = 1, + MaybeDirty = 2, + Dirty = 3, } diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index a41cd4986f6..91d9105af20 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -77,6 +77,7 @@ export class ReactiveEffect { public get dirty() { if (this._dirtyLevel === DirtyLevels.MaybeDirty) { + this._dirtyLevel = DirtyLevels.QueryingDirty pauseTracking() for (let i = 0; i < this._depsLength; i++) { const dep = this.deps[i] @@ -87,7 +88,7 @@ export class ReactiveEffect { } } } - if (this._dirtyLevel < DirtyLevels.Dirty) { + if (this._dirtyLevel === DirtyLevels.QueryingDirty) { this._dirtyLevel = DirtyLevels.NotDirty } resetTracking() @@ -140,7 +141,7 @@ function preCleanupEffect(effect: ReactiveEffect) { } function postCleanupEffect(effect: ReactiveEffect) { - if (effect.deps && effect.deps.length > effect._depsLength) { + if (effect.deps.length > effect._depsLength) { for (let i = effect._depsLength; i < effect.deps.length; i++) { cleanupDepEffect(effect.deps[i], effect) } @@ -291,35 +292,30 @@ export function triggerEffects( ) { pauseScheduling() 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 ( effect._dirtyLevel < dirtyLevel && - dep.get(effect) === effect._trackId + (tracking ??= dep.get(effect) === effect._trackId) ) { - const lastDirtyLevel = effect._dirtyLevel + effect._shouldSchedule ||= effect._dirtyLevel === DirtyLevels.NotDirty effect._dirtyLevel = dirtyLevel - if (lastDirtyLevel === DirtyLevels.NotDirty) { - effect._shouldSchedule = true - if (__DEV__) { - effect.onTrigger?.(extend({ effect }, debuggerEventExtraInfo)) - } - effect.trigger() - } } - } - scheduleEffects(dep) - resetScheduling() -} - -export function scheduleEffects(dep: Dep) { - for (const effect of dep.keys()) { if ( - effect.scheduler && effect._shouldSchedule && - (!effect._runnings || effect.allowRecurse) && - dep.get(effect) === effect._trackId + (tracking ??= dep.get(effect) === effect._trackId) ) { - effect._shouldSchedule = false - queueEffectSchedulers.push(effect.scheduler) + if (__DEV__) { + effect.onTrigger?.(extend({ effect }, debuggerEventExtraInfo)) + } + effect.trigger() + if (!effect._runnings || effect.allowRecurse) { + effect._shouldSchedule = false + if (effect.scheduler) { + queueEffectSchedulers.push(effect.scheduler) + } + } } } + resetScheduling() }