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

fix(reactivity): handle MaybeDirty recurse #10187

Merged
merged 8 commits into from
Feb 6, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/reactivity/__tests__/computed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
isReadonly,
reactive,
ref,
shallowRef,
toRaw,
} from '../src'
import { DirtyLevels } from '../src/constants'
Expand Down Expand Up @@ -521,6 +522,45 @@ 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
johnsoncodehk marked this conversation as resolved.
Show resolved Hide resolved
v2.value = true
expect(c2.effect._dirtyLevel).toBe(DirtyLevels.Dirty)
expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty)

c3.value
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')
})

it('should be not dirty after deps mutate (mutate deps in computed)', async () => {
const state = reactive<any>({})
const consumer = computed(() => {
Expand Down
12 changes: 6 additions & 6 deletions packages/reactivity/src/computed.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -44,7 +44,6 @@ export class ComputedRefImpl<T> {
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
Expand All @@ -54,10 +53,11 @@ export class ComputedRefImpl<T> {
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) {
Expand Down
5 changes: 3 additions & 2 deletions packages/reactivity/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export enum ReactiveFlags {

export enum DirtyLevels {
NotDirty = 0,
MaybeDirty = 1,
Dirty = 2,
QueryingDirty = 1,
MaybeDirty = 2,
Dirty = 3,
}
42 changes: 19 additions & 23 deletions packages/reactivity/src/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export class ReactiveEffect<T = any> {

public get dirty() {
if (this._dirtyLevel === DirtyLevels.MaybeDirty) {
this._dirtyLevel = DirtyLevels.QueryingDirty
pauseTracking()
for (let i = 0; i < this._depsLength; i++) {
const dep = this.deps[i]
Expand All @@ -87,7 +88,7 @@ export class ReactiveEffect<T = any> {
}
}
}
if (this._dirtyLevel < DirtyLevels.Dirty) {
if (this._dirtyLevel === DirtyLevels.QueryingDirty) {
this._dirtyLevel = DirtyLevels.NotDirty
}
resetTracking()
Expand Down Expand Up @@ -140,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)
}
Expand Down Expand Up @@ -291,35 +292,30 @@ export function triggerEffects(
) {
pauseScheduling()
for (const effect of dep.keys()) {
// dep.get(effect) is very expensive, we need to calculate it lazily and reuse the result
let tracking: boolean | undefined
if (
effect._dirtyLevel < dirtyLevel &&
dep.get(effect) === effect._trackId
(tracking ??= dep.get(effect) === effect._trackId)
) {
const lastDirtyLevel = effect._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()
}
}
}
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
(tracking ??= dep.get(effect) === effect._trackId)
) {
effect._shouldSchedule = false
queueEffectSchedulers.push(effect.scheduler)
if (__DEV__) {
effect.onTrigger?.(extend({ effect }, debuggerEventExtraInfo))
}
effect.trigger()
if (!effect._runnings || effect.allowRecurse) {
effect._shouldSchedule = false
if (effect.scheduler) {
queueEffectSchedulers.push(effect.scheduler)
}
}
}
}
resetScheduling()
}
Loading