From 56cae702492c3ffc80eca03fe1b4f3be73605dde Mon Sep 17 00:00:00 2001 From: daiwei Date: Mon, 21 Oct 2024 10:09:43 +0800 Subject: [PATCH 1/3] feat(computed): warn computed self recursion --- .../reactivity/__tests__/computed.spec.ts | 19 +++++++++++-------- packages/reactivity/src/computed.ts | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 123df44f253..68e3d44e4f0 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -26,7 +26,11 @@ import { triggerRef, } from '../src' import { EffectFlags, pauseTracking, resetTracking } from '../src/effect' -import type { ComputedRef, ComputedRefImpl } from '../src/computed' +import { + COMPUTED_SIDE_EFFECT_WARN, + type ComputedRef, + type ComputedRefImpl, +} from '../src/computed' describe('reactivity/computed', () => { it('should return updated value', () => { @@ -482,7 +486,6 @@ describe('reactivity/computed', () => { const c3 = computed(() => c2.value) c3.value - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should work when chained(ref+computed)', () => { @@ -496,7 +499,6 @@ describe('reactivity/computed', () => { const c2 = computed(() => v.value + c1.value) expect(c2.value).toBe('0foo') expect(c2.value).toBe('1foo') - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should trigger effect even computed already dirty', () => { @@ -520,7 +522,7 @@ describe('reactivity/computed', () => { expect(fnSpy).toBeCalledTimes(2) expect(fnSpy.mock.calls).toMatchObject([['0foo'], ['2foo']]) expect(v.value).toBe(2) - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) // #10185 @@ -551,7 +553,6 @@ describe('reactivity/computed', () => { v1.value.v.value = 999 expect(c3.value).toBe('yes') - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should be not dirty after deps mutate (mutate deps in computed)', async () => { @@ -573,7 +574,7 @@ describe('reactivity/computed', () => { await nextTick() await nextTick() expect(serializeInner(root)).toBe(`2`) - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should not trigger effect scheduler by recursive computed effect', async () => { @@ -596,7 +597,7 @@ describe('reactivity/computed', () => { v.value += ' World' await nextTick() expect(serializeInner(root)).toBe('Hello World World World World') - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) test('should not trigger if value did not change', () => { @@ -895,6 +896,7 @@ describe('reactivity/computed', () => { expect(serializeInner(root)).toBe( 'Hello World World World World | Hello World World World World', ) + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should keep dirty level when side effect computed value changed', () => { @@ -920,6 +922,7 @@ describe('reactivity/computed', () => { expect(d.value.d).toBe(1) expect(serializeInner(root)).toBe('11') + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should be recomputed without being affected by side effects', () => { @@ -935,7 +938,6 @@ describe('reactivity/computed', () => { expect(c2.value).toBe('0,0') v.value = 1 expect(c2.value).toBe('1,0') - // expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('debug: onTrigger (ref)', () => { @@ -1004,6 +1006,7 @@ describe('reactivity/computed', () => { triggerEvent(root.children[1] as TestElement, 'click') await nextTick() expect(serializeInner(root)).toBe(`

Step 2

`) + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) test('manual trigger computed', () => { diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index ea798e201d4..7910fdbee72 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -40,6 +40,12 @@ export interface WritableComputedOptions { set: ComputedSetter } +export const COMPUTED_SIDE_EFFECT_WARN: string = + `Computed has side effects,` + + ` likely because a computed is mutating its own dependency in its getter.` + + ` State mutations in computed getters should be avoided. ` + + ` Check the docs for more details: https://vuejs.org/guide/essentials/computed.html#getters-should-be-side-effect-free` + /** * @private exported by @vue/reactivity for Vue core use, but not exported from * the main vue package @@ -123,8 +129,12 @@ export class ComputedRefImpl implements Subscriber { ) { batch(this, true) return true - } else if (__DEV__) { - // TODO warn + } else if ( + __DEV__ && + activeSub === this && + (this._warnRecursive || __TEST__) + ) { + warn(COMPUTED_SIDE_EFFECT_WARN, `\n\ngetter: `, this.fn) } } From ad255b524e02298f6a0b3003fb3705c7af707633 Mon Sep 17 00:00:00 2001 From: daiwei Date: Mon, 21 Oct 2024 10:14:51 +0800 Subject: [PATCH 2/3] chore: update --- packages/reactivity/__tests__/computed.spec.ts | 14 +++++++------- packages/reactivity/src/computed.ts | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index 68e3d44e4f0..98e655ee26f 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -27,7 +27,7 @@ import { } from '../src' import { EffectFlags, pauseTracking, resetTracking } from '../src/effect' import { - COMPUTED_SIDE_EFFECT_WARN, + COMPUTED_SELF_RECURSIVE_WARN, type ComputedRef, type ComputedRefImpl, } from '../src/computed' @@ -522,7 +522,7 @@ describe('reactivity/computed', () => { expect(fnSpy).toBeCalledTimes(2) expect(fnSpy.mock.calls).toMatchObject([['0foo'], ['2foo']]) expect(v.value).toBe(2) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) // #10185 @@ -574,7 +574,7 @@ describe('reactivity/computed', () => { await nextTick() await nextTick() expect(serializeInner(root)).toBe(`2`) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) it('should not trigger effect scheduler by recursive computed effect', async () => { @@ -597,7 +597,7 @@ describe('reactivity/computed', () => { v.value += ' World' await nextTick() expect(serializeInner(root)).toBe('Hello World World World World') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) test('should not trigger if value did not change', () => { @@ -896,7 +896,7 @@ describe('reactivity/computed', () => { expect(serializeInner(root)).toBe( 'Hello World World World World | Hello World World World World', ) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) it('should keep dirty level when side effect computed value changed', () => { @@ -922,7 +922,7 @@ describe('reactivity/computed', () => { expect(d.value.d).toBe(1) expect(serializeInner(root)).toBe('11') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) it('should be recomputed without being affected by side effects', () => { @@ -1006,7 +1006,7 @@ describe('reactivity/computed', () => { triggerEvent(root.children[1] as TestElement, 'click') await nextTick() expect(serializeInner(root)).toBe(`

Step 2

`) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(COMPUTED_SELF_RECURSIVE_WARN).toHaveBeenWarned() }) test('manual trigger computed', () => { diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 7910fdbee72..326d188f45e 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -40,11 +40,11 @@ export interface WritableComputedOptions { set: ComputedSetter } -export const COMPUTED_SIDE_EFFECT_WARN: string = - `Computed has side effects,` + +export const COMPUTED_SELF_RECURSIVE_WARN: string = + `Computed has a self-recursive issue,` + ` likely because a computed is mutating its own dependency in its getter.` + ` State mutations in computed getters should be avoided. ` + - ` Check the docs for more details: https://vuejs.org/guide/essentials/computed.html#getters-should-be-side-effect-free` + ` Check the documentation for more details: https://vuejs.org/guide/essentials/computed.html#getters-should-be-side-effect-free` /** * @private exported by @vue/reactivity for Vue core use, but not exported from @@ -134,7 +134,7 @@ export class ComputedRefImpl implements Subscriber { activeSub === this && (this._warnRecursive || __TEST__) ) { - warn(COMPUTED_SIDE_EFFECT_WARN, `\n\ngetter: `, this.fn) + warn(COMPUTED_SELF_RECURSIVE_WARN, `\n\ngetter: `, this.fn) } } From aa616fa98c1f5712ffcef7717cbb4dd30139f5a3 Mon Sep 17 00:00:00 2001 From: daiwei Date: Mon, 21 Oct 2024 11:45:55 +0800 Subject: [PATCH 3/3] chore: remove TODO --- packages/runtime-core/src/apiCreateApp.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/runtime-core/src/apiCreateApp.ts b/packages/runtime-core/src/apiCreateApp.ts index 3d53716de08..6464ba8411b 100644 --- a/packages/runtime-core/src/apiCreateApp.ts +++ b/packages/runtime-core/src/apiCreateApp.ts @@ -150,7 +150,6 @@ export interface AppConfig { isCustomElement?: (tag: string) => boolean /** - * TODO document for 3.5 * Enable warnings for computed getters that recursively trigger itself. */ warnRecursiveComputed?: boolean