From 1faaca00f0fcc936df12f383c55094234323c4f0 Mon Sep 17 00:00:00 2001 From: Doctorwu Date: Fri, 9 Feb 2024 14:00:08 +0800 Subject: [PATCH 1/4] feat(reactivity): warn users when computeds are not side-effect free --- packages/reactivity/__tests__/computed.spec.ts | 7 +++++++ packages/reactivity/src/computed.ts | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index c3d0c7f15ed..c9f47720edd 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -14,6 +14,7 @@ import { toRaw, } from '../src' import { DirtyLevels } from '../src/constants' +import { COMPUTED_SIDE_EFFECT_WARN } from '../src/computed' describe('reactivity/computed', () => { it('should return updated value', () => { @@ -488,6 +489,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe( DirtyLevels.MaybeDirty_ComputedSideEffect, ) + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should work when chained(ref+computed)', () => { @@ -502,6 +504,7 @@ describe('reactivity/computed', () => { expect(c2.value).toBe('0foo') expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.value).toBe('1foo') + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should trigger effect even computed already dirty', () => { @@ -524,6 +527,7 @@ describe('reactivity/computed', () => { expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) v.value = 2 expect(fnSpy).toBeCalledTimes(2) + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) // #10185 @@ -567,6 +571,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) expect(c3.value).toBe('yes') + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should be not dirty after deps mutate (mutate deps in computed)', async () => { @@ -588,6 +593,7 @@ describe('reactivity/computed', () => { await nextTick() await nextTick() expect(serializeInner(root)).toBe(`2`) + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should not trigger effect scheduler by recurse computed effect', async () => { @@ -610,5 +616,6 @@ describe('reactivity/computed', () => { v.value += ' World' await nextTick() 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 259d4e32c8a..814e7593bf1 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -4,6 +4,7 @@ import { NOOP, hasChanged, isFunction } from '@vue/shared' import { toRaw } from './reactive' import type { Dep } from './dep' import { DirtyLevels, ReactiveFlags } from './constants' +import { warn } from './warning' declare const ComputedRefSymbol: unique symbol @@ -24,6 +25,11 @@ export interface WritableComputedOptions { set: ComputedSetter } +export const COMPUTED_SIDE_EFFECT_WARN = + `[Vue warn] Computed keep dirty after evaluation.` + + ` This might happen when you mutate the deps of computed in the getter.` + + ` Check the docs for more details: https://vuejs.org/guide/essentials/computed.html#getters-should-be-side-effect-free` + export class ComputedRefImpl { public dep?: Dep = undefined @@ -67,6 +73,7 @@ export class ComputedRefImpl { } trackRefValue(self) if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty_ComputedSideEffect) { + warn(COMPUTED_SIDE_EFFECT_WARN) triggerRefValue(self, DirtyLevels.MaybeDirty_ComputedSideEffect) } return self._value @@ -141,7 +148,7 @@ export function computed( getter = getterOrOptions setter = __DEV__ ? () => { - console.warn('Write operation failed: computed value is readonly') + warn('Write operation failed: computed value is readonly') } : NOOP } else { From 745b73a54d87263260f47bead9891c9e6f2ee9dd Mon Sep 17 00:00:00 2001 From: Doctorwu Date: Fri, 9 Feb 2024 14:05:11 +0800 Subject: [PATCH 2/4] fix: only warn in dev --- packages/reactivity/src/computed.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 814e7593bf1..a73f0a87fd8 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -73,7 +73,7 @@ export class ComputedRefImpl { } trackRefValue(self) if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty_ComputedSideEffect) { - warn(COMPUTED_SIDE_EFFECT_WARN) + __DEV__ && warn(COMPUTED_SIDE_EFFECT_WARN) triggerRefValue(self, DirtyLevels.MaybeDirty_ComputedSideEffect) } return self._value From 97b2f87767b6c7e0bcd075db72adb373a459ab21 Mon Sep 17 00:00:00 2001 From: Doctorwu Date: Sun, 11 Feb 2024 20:40:08 +0800 Subject: [PATCH 3/4] chore: prevent duplicate copyright --- packages/reactivity/__tests__/computed.spec.ts | 14 ++++++++------ packages/reactivity/src/computed.ts | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index c9f47720edd..d2f74872762 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -16,6 +16,8 @@ import { import { DirtyLevels } from '../src/constants' import { COMPUTED_SIDE_EFFECT_WARN } from '../src/computed' +const normalizedSideEffectWarn = `[Vue warn] ${COMPUTED_SIDE_EFFECT_WARN}` + describe('reactivity/computed', () => { it('should return updated value', () => { const value = reactive<{ foo?: number }>({}) @@ -489,7 +491,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe( DirtyLevels.MaybeDirty_ComputedSideEffect, ) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) it('should work when chained(ref+computed)', () => { @@ -504,7 +506,7 @@ describe('reactivity/computed', () => { expect(c2.value).toBe('0foo') expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.value).toBe('1foo') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) it('should trigger effect even computed already dirty', () => { @@ -527,7 +529,7 @@ describe('reactivity/computed', () => { expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) v.value = 2 expect(fnSpy).toBeCalledTimes(2) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) // #10185 @@ -571,7 +573,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) expect(c3.value).toBe('yes') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) it('should be not dirty after deps mutate (mutate deps in computed)', async () => { @@ -593,7 +595,7 @@ describe('reactivity/computed', () => { await nextTick() await nextTick() expect(serializeInner(root)).toBe(`2`) - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) it('should not trigger effect scheduler by recurse computed effect', async () => { @@ -616,6 +618,6 @@ describe('reactivity/computed', () => { v.value += ' World' await nextTick() expect(serializeInner(root)).toBe('Hello World World World World') - expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() + expect(normalizedSideEffectWarn).toHaveBeenWarned() }) }) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index a73f0a87fd8..736c6a10739 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -26,7 +26,7 @@ export interface WritableComputedOptions { } export const COMPUTED_SIDE_EFFECT_WARN = - `[Vue warn] Computed keep dirty after evaluation.` + + `Computed keep dirty after evaluation.` + ` This might happen when you mutate the deps of computed in the getter.` + ` Check the docs for more details: https://vuejs.org/guide/essentials/computed.html#getters-should-be-side-effect-free` From 43e2ea86a9c2b9affd26c80fa7793858625808c6 Mon Sep 17 00:00:00 2001 From: Evan You Date: Tue, 13 Feb 2024 17:33:35 +0800 Subject: [PATCH 4/4] chore: adjust warning --- packages/reactivity/__tests__/computed.spec.ts | 14 ++++++-------- packages/reactivity/src/computed.ts | 5 +++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts index d2f74872762..c9f47720edd 100644 --- a/packages/reactivity/__tests__/computed.spec.ts +++ b/packages/reactivity/__tests__/computed.spec.ts @@ -16,8 +16,6 @@ import { import { DirtyLevels } from '../src/constants' import { COMPUTED_SIDE_EFFECT_WARN } from '../src/computed' -const normalizedSideEffectWarn = `[Vue warn] ${COMPUTED_SIDE_EFFECT_WARN}` - describe('reactivity/computed', () => { it('should return updated value', () => { const value = reactive<{ foo?: number }>({}) @@ -491,7 +489,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe( DirtyLevels.MaybeDirty_ComputedSideEffect, ) - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should work when chained(ref+computed)', () => { @@ -506,7 +504,7 @@ describe('reactivity/computed', () => { expect(c2.value).toBe('0foo') expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) expect(c2.value).toBe('1foo') - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should trigger effect even computed already dirty', () => { @@ -529,7 +527,7 @@ describe('reactivity/computed', () => { expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty) v.value = 2 expect(fnSpy).toBeCalledTimes(2) - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) // #10185 @@ -573,7 +571,7 @@ describe('reactivity/computed', () => { expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty) expect(c3.value).toBe('yes') - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should be not dirty after deps mutate (mutate deps in computed)', async () => { @@ -595,7 +593,7 @@ describe('reactivity/computed', () => { await nextTick() await nextTick() expect(serializeInner(root)).toBe(`2`) - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) it('should not trigger effect scheduler by recurse computed effect', async () => { @@ -618,6 +616,6 @@ describe('reactivity/computed', () => { v.value += ' World' await nextTick() expect(serializeInner(root)).toBe('Hello World World World World') - expect(normalizedSideEffectWarn).toHaveBeenWarned() + expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned() }) }) diff --git a/packages/reactivity/src/computed.ts b/packages/reactivity/src/computed.ts index 736c6a10739..a4b74172fcf 100644 --- a/packages/reactivity/src/computed.ts +++ b/packages/reactivity/src/computed.ts @@ -26,8 +26,9 @@ export interface WritableComputedOptions { } export const COMPUTED_SIDE_EFFECT_WARN = - `Computed keep dirty after evaluation.` + - ` This might happen when you mutate the deps of computed in the getter.` + + `Computed is still dirty after getter evaluation,` + + ` 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` export class ComputedRefImpl {