From 82baeefd2cabdec1417b6cdd433846e6d38e0192 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 23 Jan 2024 09:43:24 +0800 Subject: [PATCH 1/7] fix(reactivity): handle `MaybeDirty` recurse --- packages/reactivity/src/computed.ts | 3 +- packages/reactivity/src/effect.ts | 46 +++++++++++------------------ 2 files changed, 18 insertions(+), 31 deletions(-) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 03459c7dfb6..7c3fb23ff4b 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 diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index a41cd4986f6..4dacbad241c 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -76,7 +76,8 @@ export class ReactiveEffect { } public get dirty() { - if (this._dirtyLevel === DirtyLevels.MaybeDirty) { + while (this._dirtyLevel === DirtyLevels.MaybeDirty) { + this._dirtyLevel = DirtyLevels.NotDirty pauseTracking() for (let i = 0; i < this._depsLength; i++) { const dep = this.deps[i] @@ -87,9 +88,6 @@ export class ReactiveEffect { } } } - if (this._dirtyLevel < DirtyLevels.Dirty) { - this._dirtyLevel = DirtyLevels.NotDirty - } resetTracking() } return this._dirtyLevel >= DirtyLevels.Dirty @@ -291,35 +289,25 @@ export function triggerEffects( ) { pauseScheduling() for (const effect of dep.keys()) { - if ( - effect._dirtyLevel < dirtyLevel && - dep.get(effect) === effect._trackId - ) { - const lastDirtyLevel = effect._dirtyLevel + if (dep.get(effect) !== effect._trackId) { + continue + } + if (effect._dirtyLevel < 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() + } + if (effect._shouldSchedule) { + 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 - ) { + if (effect._shouldSchedule && (!effect._runnings || effect.allowRecurse)) { effect._shouldSchedule = false - queueEffectSchedulers.push(effect.scheduler) + if (effect.scheduler) { + queueEffectSchedulers.push(effect.scheduler) + } } } + resetScheduling() } From 1e94cfb298c8419e734586253b7e55bb822f9d35 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 23 Jan 2024 11:56:02 +0800 Subject: [PATCH 2/7] chore: add test --- .../reactivity/__tests__/computed.spec.ts | 31 +++++++++++++++++++ packages/reactivity/src/constants.ts | 5 +-- packages/reactivity/src/effect.ts | 7 +++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 860e4dab154..47ec08ac030 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -11,6 +11,7 @@ import { reactive, ref, toRaw, + shallowRef, } from '../src' import { DirtyLevels } from '../src/constants' @@ -521,6 +522,36 @@ 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 + c3.value + v1.value.v.value = 999 + + 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/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 4dacbad241c..bdcd2d9c96b 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -76,8 +76,8 @@ export class ReactiveEffect { } public get dirty() { - while (this._dirtyLevel === DirtyLevels.MaybeDirty) { - this._dirtyLevel = DirtyLevels.NotDirty + if (this._dirtyLevel === DirtyLevels.MaybeDirty) { + this._dirtyLevel = DirtyLevels.QueryingDirty pauseTracking() for (let i = 0; i < this._depsLength; i++) { const dep = this.deps[i] @@ -88,6 +88,9 @@ export class ReactiveEffect { } } } + if (this._dirtyLevel === DirtyLevels.QueryingDirty) { + this._dirtyLevel = DirtyLevels.NotDirty + } resetTracking() } return this._dirtyLevel >= DirtyLevels.Dirty From 31947664357222003471c0a131f5ec5e10308ace Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 23 Jan 2024 03:57:04 +0000 Subject: [PATCH 3/7] [autofix.ci] apply automated fixes --- packages/reactivity/__tests__/computed.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 47ec08ac030..21aeb6f984f 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -10,8 +10,8 @@ import { isReadonly, reactive, ref, - toRaw, shallowRef, + toRaw, } from '../src' import { DirtyLevels } from '../src/constants' From b646f38103d7b151630001eeac6bf244a2306cc7 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 23 Jan 2024 12:24:51 +0800 Subject: [PATCH 4/7] perf: delay `dep.get(effect)` calculate --- packages/reactivity/src/effect.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index bdcd2d9c96b..e135fc33f0f 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -292,23 +292,28 @@ export function triggerEffects( ) { pauseScheduling() for (const effect of dep.keys()) { - if (dep.get(effect) !== effect._trackId) { - continue - } - if (effect._dirtyLevel < dirtyLevel) { + // dep.get(effect) is very expensive, we need to calculate it lazily and reuse the result + let tracking: boolean | undefined + if ( + effect._dirtyLevel < dirtyLevel && + (tracking ??= dep.get(effect) === effect._trackId) + ) { effect._shouldSchedule ||= effect._dirtyLevel === DirtyLevels.NotDirty effect._dirtyLevel = dirtyLevel } - if (effect._shouldSchedule) { + if ( + effect._shouldSchedule && + (tracking ??= dep.get(effect) === effect._trackId) + ) { if (__DEV__) { effect.onTrigger?.(extend({ effect }, debuggerEventExtraInfo)) } effect.trigger() - } - if (effect._shouldSchedule && (!effect._runnings || effect.allowRecurse)) { - effect._shouldSchedule = false - if (effect.scheduler) { - queueEffectSchedulers.push(effect.scheduler) + if (!effect._runnings || effect.allowRecurse) { + effect._shouldSchedule = false + if (effect.scheduler) { + queueEffectSchedulers.push(effect.scheduler) + } } } } From 80547d744ebfdb67553b82cd1fb175ad93359efb Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 23 Jan 2024 12:49:44 +0800 Subject: [PATCH 5/7] chore: remove unneeded condition --- packages/reactivity/src/effect.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index e135fc33f0f..91d9105af20 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -141,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) } From 1c537761c06b8aa3f3b4cfde3e77fa0a1feaf1d9 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Tue, 23 Jan 2024 16:19:31 +0800 Subject: [PATCH 6/7] Update computed.spec.ts --- packages/reactivity/__tests__/computed.spec.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 21aeb6f984f..5a0beb973e8 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -545,10 +545,19 @@ describe('reactivity/computed', () => { c3.value v2.value = true + expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) + expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) + c3.value - 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) + 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') }) From 3b37ba12a2f7cd9641cc6be0ac54e850d154fd59 Mon Sep 17 00:00:00 2001 From: Johnson Chu Date: Mon, 29 Jan 2024 18:52:29 +0800 Subject: [PATCH 7/7] chore: format --- packages/reactivity/src/computed.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 7c3fb23ff4b..9eed5bc8399 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -53,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) {