From 4a0385ebbc94b0e07b9fefb430942ffc1b254ca1 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 09:46:34 +0800 Subject: [PATCH 1/7] fix(reactivity): force trigger computed --- packages/reactivity/src/effect.ts | 8 +++++--- packages/reactivity/src/ref.ts | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 886f380dd52..809b6d7499e 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -49,6 +49,7 @@ export enum EffectFlags { DIRTY = 1 << 4, ALLOW_RECURSE = 1 << 5, PAUSED = 1 << 6, + FORCE_TRIGGER = 1 << 7, } /** @@ -364,7 +365,8 @@ function isDirty(sub: Subscriber): boolean { export function refreshComputed(computed: ComputedRefImpl): undefined { if ( computed.flags & EffectFlags.TRACKING && - !(computed.flags & EffectFlags.DIRTY) + !(computed.flags & EffectFlags.DIRTY) && + !(computed.flags & EffectFlags.FORCE_TRIGGER) ) { return } @@ -386,13 +388,12 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { if ( dep.version > 0 && !computed.isSSR && - computed.deps && + !(computed.flags & EffectFlags.FORCE_TRIGGER) && !isDirty(computed) ) { computed.flags &= ~EffectFlags.RUNNING return } - const prevSub = activeSub const prevShouldTrack = shouldTrack activeSub = computed @@ -413,6 +414,7 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { shouldTrack = prevShouldTrack cleanupDeps(computed) computed.flags &= ~EffectFlags.RUNNING + computed.flags &= ~EffectFlags.FORCE_TRIGGER } } diff --git a/packages/reactivity/src/ref.ts b/packages/reactivity/src/ref.ts index 6b8d541819d..20539ba9bed 100644 --- a/packages/reactivity/src/ref.ts +++ b/packages/reactivity/src/ref.ts @@ -19,6 +19,7 @@ import { import type { ComputedRef, WritableComputedRef } from './computed' import { ReactiveFlags, TrackOpTypes, TriggerOpTypes } from './constants' import { warn } from './warning' +import { EffectFlags } from './effect' declare const RefSymbol: unique symbol export declare const RawSymbol: unique symbol @@ -186,6 +187,9 @@ class RefImpl { export function triggerRef(ref: Ref): void { // ref may be an instance of ObjectRefImpl if ((ref as unknown as RefImpl).dep) { + if ((ref as unknown as RefImpl).dep.computed) { + ;(ref as any).flags |= EffectFlags.FORCE_TRIGGER + } if (__DEV__) { ;(ref as unknown as RefImpl).dep.trigger({ target: ref, From d3c90e84c47d37de4ff5656df4c2a2f387de5c60 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 11:56:32 +0800 Subject: [PATCH 2/7] test: add test case --- .../reactivity/__tests__/computed.spec.ts | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 123df44f253..ea5edef1850 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -1012,6 +1012,42 @@ describe('reactivity/computed', () => { expect(cValue.value).toBe(1) }) + test('should not recompute when condition changes if computed does not track reactive data', async () => { + const spy = vi.fn() + const lookup = computed(() => { + spy() + return { + g: 'foo', + } + }) + + const unit = ref('') + const display = computed(() => { + if (!unit.value) return 'empty' + // @ts-expect-error + return lookup.value[unit.value] + }) + + function toggle() { + unit.value = unit.value === 'g' ? '' : 'g' + } + + expect(display.value).toBe('empty') + expect(spy).toHaveBeenCalledTimes(0) + + toggle() + expect(display.value).toBe('foo') + expect(spy).toHaveBeenCalledTimes(1) + + toggle() + expect(display.value).toBe('empty') + expect(spy).toHaveBeenCalledTimes(1) + + toggle() + expect(display.value).toBe('foo') + expect(spy).toHaveBeenCalledTimes(1) + }) + test('computed should remain live after losing all subscribers', () => { const state = reactive({ a: 1 }) const p = computed(() => state.a + 1) From adccdee161af67406da20788a716355ed35ee8d9 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 14:27:08 +0800 Subject: [PATCH 3/7] chore: improve code --- .../reactivity/__tests__/computed.spec.ts | 37 +++---------------- packages/reactivity/src/effect.ts | 17 ++++----- packages/reactivity/src/ref.ts | 4 -- 3 files changed, 14 insertions(+), 44 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index ea5edef1850..c7963499f85 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -1012,40 +1012,15 @@ describe('reactivity/computed', () => { expect(cValue.value).toBe(1) }) - test('should not recompute when condition changes if computed does not track reactive data', async () => { + test('should not recompute if computed does not track reactive data', async () => { const spy = vi.fn() - const lookup = computed(() => { - spy() - return { - g: 'foo', - } - }) - - const unit = ref('') - const display = computed(() => { - if (!unit.value) return 'empty' - // @ts-expect-error - return lookup.value[unit.value] - }) - - function toggle() { - unit.value = unit.value === 'g' ? '' : 'g' - } - - expect(display.value).toBe('empty') - expect(spy).toHaveBeenCalledTimes(0) - - toggle() - expect(display.value).toBe('foo') - expect(spy).toHaveBeenCalledTimes(1) + const c1 = computed(() => spy()) - toggle() - expect(display.value).toBe('empty') - expect(spy).toHaveBeenCalledTimes(1) + c1.value + ref(0).value++ // update globalVersion + c1.value - toggle() - expect(display.value).toBe('foo') - expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toBeCalledTimes(1) }) test('computed should remain live after losing all subscribers', () => { diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 809b6d7499e..1fae31d0520 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -49,7 +49,7 @@ export enum EffectFlags { DIRTY = 1 << 4, ALLOW_RECURSE = 1 << 5, PAUSED = 1 << 6, - FORCE_TRIGGER = 1 << 7, + EVALUATED = 1 << 7, } /** @@ -365,8 +365,7 @@ function isDirty(sub: Subscriber): boolean { export function refreshComputed(computed: ComputedRefImpl): undefined { if ( computed.flags & EffectFlags.TRACKING && - !(computed.flags & EffectFlags.DIRTY) && - !(computed.flags & EffectFlags.FORCE_TRIGGER) + !(computed.flags & EffectFlags.DIRTY) ) { return } @@ -379,21 +378,21 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { } computed.globalVersion = globalVersion - const dep = computed.dep - computed.flags |= EffectFlags.RUNNING // In SSR there will be no render effect, so the computed has no subscriber // and therefore tracks no deps, thus we cannot rely on the dirty check. // Instead, computed always re-evaluate and relies on the globalVersion // fast path above for caching. if ( - dep.version > 0 && !computed.isSSR && - !(computed.flags & EffectFlags.FORCE_TRIGGER) && - !isDirty(computed) + computed.flags & EffectFlags.EVALUATED && + (!computed.deps || (computed.deps && !isDirty(computed))) ) { computed.flags &= ~EffectFlags.RUNNING return } + computed.flags |= EffectFlags.RUNNING + + const dep = computed.dep const prevSub = activeSub const prevShouldTrack = shouldTrack activeSub = computed @@ -403,6 +402,7 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { prepareDeps(computed) const value = computed.fn(computed._value) if (dep.version === 0 || hasChanged(value, computed._value)) { + computed.flags |= EffectFlags.EVALUATED computed._value = value dep.version++ } @@ -414,7 +414,6 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { shouldTrack = prevShouldTrack cleanupDeps(computed) computed.flags &= ~EffectFlags.RUNNING - computed.flags &= ~EffectFlags.FORCE_TRIGGER } } diff --git a/packages/reactivity/src/ref.ts b/packages/reactivity/src/ref.ts index 20539ba9bed..6b8d541819d 100644 --- a/packages/reactivity/src/ref.ts +++ b/packages/reactivity/src/ref.ts @@ -19,7 +19,6 @@ import { import type { ComputedRef, WritableComputedRef } from './computed' import { ReactiveFlags, TrackOpTypes, TriggerOpTypes } from './constants' import { warn } from './warning' -import { EffectFlags } from './effect' declare const RefSymbol: unique symbol export declare const RawSymbol: unique symbol @@ -187,9 +186,6 @@ class RefImpl { export function triggerRef(ref: Ref): void { // ref may be an instance of ObjectRefImpl if ((ref as unknown as RefImpl).dep) { - if ((ref as unknown as RefImpl).dep.computed) { - ;(ref as any).flags |= EffectFlags.FORCE_TRIGGER - } if (__DEV__) { ;(ref as unknown as RefImpl).dep.trigger({ target: ref, From 986c8a68fddb5ec00cb02cdcea62104d8e93abc1 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 14:40:49 +0800 Subject: [PATCH 4/7] chore: improve code --- packages/reactivity/src/effect.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts index 1fae31d0520..e5202bc319a 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -382,12 +382,13 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { // and therefore tracks no deps, thus we cannot rely on the dirty check. // Instead, computed always re-evaluate and relies on the globalVersion // fast path above for caching. + // #12337 if computed no deps (does not rely on any reactive data) and evaluated, + // there is no need to re-evaluate. if ( !computed.isSSR && computed.flags & EffectFlags.EVALUATED && (!computed.deps || (computed.deps && !isDirty(computed))) ) { - computed.flags &= ~EffectFlags.RUNNING return } computed.flags |= EffectFlags.RUNNING From e143ce8aa2316e79dc20980d2901d7684951ad14 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 14:41:58 +0800 Subject: [PATCH 5/7] chore: improve code --- 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 e5202bc319a..2271dc5d675 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -382,7 +382,7 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { // and therefore tracks no deps, thus we cannot rely on the dirty check. // Instead, computed always re-evaluate and relies on the globalVersion // fast path above for caching. - // #12337 if computed no deps (does not rely on any reactive data) and evaluated, + // #12337 if computed has no deps (does not rely on any reactive data) and evaluated, // there is no need to re-evaluate. if ( !computed.isSSR && From 08a2ac32afc4bb868457cfe1928565bd1e0f37f3 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 16:00:01 +0800 Subject: [PATCH 6/7] chore: fix pinia test failing --- 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 2271dc5d675..80fb8d09e6c 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -387,7 +387,7 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { if ( !computed.isSSR && computed.flags & EffectFlags.EVALUATED && - (!computed.deps || (computed.deps && !isDirty(computed))) + (!computed.deps || !isDirty(computed)) ) { return } From bf6500de48a8d9cb98365b546ab325a9f667d313 Mon Sep 17 00:00:00 2001 From: daiwei Date: Thu, 7 Nov 2024 16:15:20 +0800 Subject: [PATCH 7/7] chore: fix pinia test failing --- 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 80fb8d09e6c..84cf184c154 100644 --- a/packages/reactivity/src/effect.ts +++ b/packages/reactivity/src/effect.ts @@ -387,7 +387,7 @@ export function refreshComputed(computed: ComputedRefImpl): undefined { if ( !computed.isSSR && computed.flags & EffectFlags.EVALUATED && - (!computed.deps || !isDirty(computed)) + ((!computed.deps && !(computed as any)._dirty) || !isDirty(computed)) ) { return }