From: Evan You <evan@vuejs.org>
Date: Thu, 26 Sep 2024 08:58:38 +0000 (+0800)
Subject: fix(reactivity): fix property dep removal regression
X-Git-Tag: v3.5.9~6
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=6001e5c81a05c894586f9287fbd991677bdd0455;p=thirdparty%2Fvuejs%2Fcore.git

fix(reactivity): fix property dep removal regression

close #12020
close #12021
---

diff --git a/packages/reactivity/__tests__/computed.spec.ts b/packages/reactivity/__tests__/computed.spec.ts
index e4a337cb8b..8e9b048e6f 100644
--- a/packages/reactivity/__tests__/computed.spec.ts
+++ b/packages/reactivity/__tests__/computed.spec.ts
@@ -1023,6 +1023,7 @@ describe('reactivity/computed', () => {
     expect(p.value).toBe(3)
   })
 
+  // #11995
   test('computed dep cleanup should not cause property dep to be deleted', () => {
     const toggle = ref(true)
     const state = reactive({ a: 1 })
@@ -1037,4 +1038,23 @@ describe('reactivity/computed', () => {
     state.a++
     expect(pp.value).toBe(2)
   })
+
+  // #12020
+  test('computed value updates correctly after dep cleanup', () => {
+    const obj = reactive({ foo: 1, flag: 1 })
+    const c1 = computed(() => obj.foo)
+
+    let foo
+    effect(() => {
+      foo = obj.flag ? (obj.foo, c1.value) : 0
+    })
+    expect(foo).toBe(1)
+
+    obj.flag = 0
+    expect(foo).toBe(0)
+
+    obj.foo = 2
+    obj.flag = 1
+    expect(foo).toBe(2)
+  })
 })
diff --git a/packages/reactivity/__tests__/reactive.spec.ts b/packages/reactivity/__tests__/reactive.spec.ts
index 3b3baa1906..47f6aa297e 100644
--- a/packages/reactivity/__tests__/reactive.spec.ts
+++ b/packages/reactivity/__tests__/reactive.spec.ts
@@ -13,6 +13,7 @@ import {
 } from '../src/reactive'
 import { computed } from '../src/computed'
 import { effect } from '../src/effect'
+import { targetMap } from '../src/dep'
 
 describe('reactivity/reactive', () => {
   test('Object', () => {
@@ -398,4 +399,14 @@ describe('reactivity/reactive', () => {
       a.value++
     }).not.toThrow()
   })
+
+  // #11979
+  test('should release property Dep instance if it no longer has subscribers', () => {
+    let obj = { x: 1 }
+    let a = reactive(obj)
+    const e = effect(() => a.x)
+    expect(targetMap.get(obj)?.get('x')).toBeTruthy()
+    e.effect.stop()
+    expect(targetMap.get(obj)?.get('x')).toBeFalsy()
+  })
 })
diff --git a/packages/reactivity/src/dep.ts b/packages/reactivity/src/dep.ts
index 0a795be4f4..e7e9672dc9 100644
--- a/packages/reactivity/src/dep.ts
+++ b/packages/reactivity/src/dep.ts
@@ -89,6 +89,11 @@ export class Dep {
   map?: KeyToDepMap = undefined
   key?: unknown = undefined
 
+  /**
+   * Subscriber counter
+   */
+  sc: number = 0
+
   constructor(public computed?: ComputedRefImpl | undefined) {
     if (__DEV__) {
       this.subsHead = undefined
@@ -113,9 +118,7 @@ export class Dep {
         activeSub.depsTail = link
       }
 
-      if (activeSub.flags & EffectFlags.TRACKING) {
-        addSub(link)
-      }
+      addSub(link)
     } else if (link.version === -1) {
       // reused from last run - already a sub, just sync version
       link.version = this.version
@@ -197,27 +200,30 @@ export class Dep {
 }
 
 function addSub(link: Link) {
-  const computed = link.dep.computed
-  // computed getting its first subscriber
-  // enable tracking + lazily subscribe to all its deps
-  if (computed && !link.dep.subs) {
-    computed.flags |= EffectFlags.TRACKING | EffectFlags.DIRTY
-    for (let l = computed.deps; l; l = l.nextDep) {
-      addSub(l)
+  link.dep.sc++
+  if (link.sub.flags & EffectFlags.TRACKING) {
+    const computed = link.dep.computed
+    // computed getting its first subscriber
+    // enable tracking + lazily subscribe to all its deps
+    if (computed && !link.dep.subs) {
+      computed.flags |= EffectFlags.TRACKING | EffectFlags.DIRTY
+      for (let l = computed.deps; l; l = l.nextDep) {
+        addSub(l)
+      }
     }
-  }
 
-  const currentTail = link.dep.subs
-  if (currentTail !== link) {
-    link.prevSub = currentTail
-    if (currentTail) currentTail.nextSub = link
-  }
+    const currentTail = link.dep.subs
+    if (currentTail !== link) {
+      link.prevSub = currentTail
+      if (currentTail) currentTail.nextSub = link
+    }
 
-  if (__DEV__ && link.dep.subsHead === undefined) {
-    link.dep.subsHead = link
-  }
+    if (__DEV__ && link.dep.subsHead === undefined) {
+      link.dep.subsHead = link
+    }
 
-  link.dep.subs = link
+    link.dep.subs = link
+  }
 }
 
 // The main WeakMap that stores {target -> key -> dep} connections.
diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts
index 20bbada505..b681df85cd 100644
--- a/packages/reactivity/src/effect.ts
+++ b/packages/reactivity/src/effect.ts
@@ -1,7 +1,7 @@
 import { extend, hasChanged } from '@vue/shared'
 import type { ComputedRefImpl } from './computed'
 import type { TrackOpTypes, TriggerOpTypes } from './constants'
-import { type Link, globalVersion, targetMap } from './dep'
+import { type Link, globalVersion } from './dep'
 import { activeEffectScope } from './effectScope'
 import { warn } from './warning'
 
@@ -292,7 +292,7 @@ function prepareDeps(sub: Subscriber) {
   }
 }
 
-function cleanupDeps(sub: Subscriber, fromComputed = false) {
+function cleanupDeps(sub: Subscriber) {
   // Cleanup unsued deps
   let head
   let tail = sub.depsTail
@@ -302,7 +302,7 @@ function cleanupDeps(sub: Subscriber, fromComputed = false) {
     if (link.version === -1) {
       if (link === tail) tail = prev
       // unused - remove it from the dep's subscribing effect list
-      removeSub(link, fromComputed)
+      removeSub(link)
       // also remove it from this effect's dep list
       removeDep(link)
     } else {
@@ -394,12 +394,12 @@ export function refreshComputed(computed: ComputedRefImpl): undefined {
   } finally {
     activeSub = prevSub
     shouldTrack = prevShouldTrack
-    cleanupDeps(computed, true)
+    cleanupDeps(computed)
     computed.flags &= ~EffectFlags.RUNNING
   }
 }
 
-function removeSub(link: Link, fromComputed = false) {
+function removeSub(link: Link, soft = false) {
   const { dep, prevSub, nextSub } = link
   if (prevSub) {
     prevSub.nextSub = nextSub
@@ -418,21 +418,24 @@ function removeSub(link: Link, fromComputed = false) {
     dep.subsHead = nextSub
   }
 
-  if (!dep.subs) {
-    // last subscriber removed
-    if (dep.computed) {
-      // if computed, unsubscribe it from all its deps so this computed and its
-      // value can be GCed
-      dep.computed.flags &= ~EffectFlags.TRACKING
-      for (let l = dep.computed.deps; l; l = l.nextDep) {
-        removeSub(l, true)
-      }
-    } else if (dep.map && !fromComputed) {
-      // property dep, remove it from the owner depsMap
-      dep.map.delete(dep.key)
-      if (!dep.map.size) targetMap.delete(dep.target!)
+  if (!dep.subs && dep.computed) {
+    // if computed, unsubscribe it from all its deps so this computed and its
+    // value can be GCed
+    dep.computed.flags &= ~EffectFlags.TRACKING
+    for (let l = dep.computed.deps; l; l = l.nextDep) {
+      // here we are only "soft" unsubscribing because the computed still keeps
+      // referencing the deps and the dep should not decrease its sub count
+      removeSub(l, true)
     }
   }
+
+  if (!soft && !--dep.sc && dep.map) {
+    // #11979
+    // property dep no longer has effect subscribers, delete it
+    // this mostly is for the case where an object is kept in memory but only a
+    // subset of its properties is tracked at one time
+    dep.map.delete(dep.key)
+  }
 }
 
 function removeDep(link: Link) {