fix(reactivity): fix dirtyLevel checks for recursive effects (#10101)

close #10082

Author: johnsoncodehk

packages/reactivity/__tests__/computed.spec.ts

@@ -11,6 +11,7 @@ import {
   ref,
   toRaw,
 } from '../src'
+import { DirtyLevels } from '../src/constants'
 
 describe('reactivity/computed', () => {
   it('should return updated value', () => {
@@ -451,4 +452,33 @@ describe('reactivity/computed', () => {
     v.value = 2
     expect(fnSpy).toBeCalledTimes(2)
   })
+
+  it('...', () => {
+    const fnSpy = vi.fn()
+    const v = ref(1)
+    const c1 = computed(() => v.value)
+    const c2 = computed(() => {
+      fnSpy()
+      return c1.value
+    })
+
+    c1.effect.allowRecurse = true
+    c2.effect.allowRecurse = true
+    c2.value
+
+    expect(c1.effect._dirtyLevel).toBe(DirtyLevels.NotDirty)
+    expect(c2.effect._dirtyLevel).toBe(DirtyLevels.NotDirty)
+  })
+
+  it('should work when chained(ref+computed)', () => {
+    const value = ref(0)
+    const consumer = computed(() => {
+      value.value++
+      return 'foo'
+    })
+    const provider = computed(() => value.value + consumer.value)
+    expect(provider.value).toBe('0foo')
+    expect(provider.effect._dirtyLevel).toBe(DirtyLevels.Dirty)
+    expect(provider.value).toBe('1foo')
+  })
 })

packages/reactivity/__tests__/effect.spec.ts

@@ -13,6 +13,15 @@ import {
 } from '../src/index'
 import { pauseScheduling, resetScheduling } from '../src/effect'
 import { ITERATE_KEY, getDepFromReactive } from '../src/reactiveEffect'
+import {
+  computed,
+  h,
+  nextTick,
+  nodeOps,
+  ref,
+  render,
+  serializeInner,
+} from '@vue/runtime-test'
 
 describe('reactivity/effect', () => {
   it('should run the passed function once (wrapped by a effect)', () => {
@@ -1011,6 +1020,35 @@ describe('reactivity/effect', () => {
     expect(counterSpy).toHaveBeenCalledTimes(1)
   })
 
+  // #10082
+  it('should set dirtyLevel when effect is allowRecurse and is running', async () => {
+    const s = ref(0)
+    const n = computed(() => s.value + 1)
+
+    const Child = {
+      setup() {
+        s.value++
+        return () => n.value
+      },
+    }
+
+    const renderSpy = vi.fn()
+    const Parent = {
+      setup() {
+        return () => {
+          renderSpy()
+          return [n.value, h(Child)]
+        }
+      },
+    }
+
+    const root = nodeOps.createElement('div')
+    render(h(Parent), root)
+    await nextTick()
+    expect(serializeInner(root)).toBe('22')
+    expect(renderSpy).toHaveBeenCalledTimes(2)
+  })
+
   describe('empty dep cleanup', () => {
     it('should remove the dep when the effect is stopped', () => {
       const obj = reactive({ prop: 1 })

packages/reactivity/src/computed.ts

@@ -43,7 +43,7 @@ export class ComputedRefImpl<T> {
   ) {
     this.effect = new ReactiveEffect(
       () => getter(this._value),
-      () => triggerRefValue(this, DirtyLevels.ComputedValueMaybeDirty),
+      () => triggerRefValue(this, DirtyLevels.MaybeDirty),
     )
     this.effect.computed = this
     this.effect.active = this._cacheable = !isSSR
@@ -53,12 +53,12 @@ export class ComputedRefImpl<T> {
   get value() {
     // the computed ref may get wrapped by other proxies e.g. readonly() #3376
     const self = toRaw(this)
-    trackRefValue(self)
     if (!self._cacheable || self.effect.dirty) {
       if (hasChanged(self._value, (self._value = self.effect.run()!))) {
-        triggerRefValue(self, DirtyLevels.ComputedValueDirty)
+        triggerRefValue(self, DirtyLevels.Dirty)
       }
     }
+    trackRefValue(self)
     return self._value
   }
 

packages/reactivity/src/constants.ts

@@ -24,7 +24,6 @@ export enum ReactiveFlags {
 
 export enum DirtyLevels {
   NotDirty = 0,
-  ComputedValueMaybeDirty = 1,
-  ComputedValueDirty = 2,
-  Dirty = 3,
+  MaybeDirty = 1,
+  Dirty = 2,
 }

packages/reactivity/src/effect.ts

@@ -60,7 +60,7 @@ export class ReactiveEffect<T = any> {
   /**
    * @internal
    */
-  _queryings = 0
+  _shouldSchedule = false
   /**
    * @internal
    */
@@ -76,22 +76,23 @@ export class ReactiveEffect<T = any> {
   }
 
   public get dirty() {
-    if (this._dirtyLevel === DirtyLevels.ComputedValueMaybeDirty) {
-      this._dirtyLevel = DirtyLevels.NotDirty
-      this._queryings++
+    if (this._dirtyLevel === DirtyLevels.MaybeDirty) {
       pauseTracking()
-      for (const dep of this.deps) {
+      for (let i = 0; i < this._depsLength; i++) {
+        const dep = this.deps[i]
         if (dep.computed) {
           triggerComputed(dep.computed)
-          if (this._dirtyLevel >= DirtyLevels.ComputedValueDirty) {
+          if (this._dirtyLevel >= DirtyLevels.Dirty) {
             break
           }
         }
       }
+      if (this._dirtyLevel < DirtyLevels.Dirty) {
+        this._dirtyLevel = DirtyLevels.NotDirty
+      }
       resetTracking()
-      this._queryings--
     }
-    return this._dirtyLevel >= DirtyLevels.ComputedValueDirty
+    return this._dirtyLevel >= DirtyLevels.Dirty
   }
 
   public set dirty(v) {
@@ -290,28 +291,29 @@ export function triggerEffects(
 ) {
   pauseScheduling()
   for (const effect of dep.keys()) {
-    if (!effect.allowRecurse && effect._runnings) {
+    if (dep.get(effect) !== effect._trackId) {
+      // when recurse effect is running, dep map could have outdated items
       continue
     }
-    if (
-      effect._dirtyLevel < dirtyLevel &&
-      (!effect._runnings || dirtyLevel !== DirtyLevels.ComputedValueDirty)
-    ) {
+    if (effect._dirtyLevel < dirtyLevel) {
       const lastDirtyLevel = effect._dirtyLevel
       effect._dirtyLevel = dirtyLevel
-      if (
-        lastDirtyLevel === DirtyLevels.NotDirty &&
-        (!effect._queryings || dirtyLevel !== DirtyLevels.ComputedValueDirty)
-      ) {
+      if (lastDirtyLevel === DirtyLevels.NotDirty) {
+        effect._shouldSchedule = true
         if (__DEV__) {
           effect.onTrigger?.(extend({ effect }, debuggerEventExtraInfo))
         }
         effect.trigger()
-        if (effect.scheduler) {
-          queueEffectSchedulers.push(effect.scheduler)
-        }
       }
     }
+    if (
+      effect.scheduler &&
+      effect._shouldSchedule &&
+      (!effect._runnings || effect.allowRecurse)
+    ) {
+      effect._shouldSchedule = false
+      queueEffectSchedulers.push(effect.scheduler)
+    }
   }
   resetScheduling()
 }