Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(reactivity): warn users when computeds are not side-effect free #10299

Merged
merged 6 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/reactivity/__tests__/computed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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)', () => {
Expand All @@ -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', () => {
Expand All @@ -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
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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()
})
})
10 changes: 9 additions & 1 deletion packages/reactivity/src/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -24,6 +25,12 @@ export interface WritableComputedOptions<T> {
set: ComputedSetter<T>
}

export const COMPUTED_SIDE_EFFECT_WARN =
`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<T> {
public dep?: Dep = undefined

Expand Down Expand Up @@ -67,6 +74,7 @@ export class ComputedRefImpl<T> {
}
trackRefValue(self)
if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty_ComputedSideEffect) {
__DEV__ && warn(COMPUTED_SIDE_EFFECT_WARN)
triggerRefValue(self, DirtyLevels.MaybeDirty_ComputedSideEffect)
}
return self._value
Expand Down Expand Up @@ -141,7 +149,7 @@ export function computed<T>(
getter = getterOrOptions
setter = __DEV__
? () => {
console.warn('Write operation failed: computed value is readonly')
warn('Write operation failed: computed value is readonly')
}
: NOOP
} else {
Expand Down