fix(reactivity): track reactive keys in raw collection types

Also warn against presence of both raw and reactive versions of the
same object in a collection as keys.

fix #919

Author: yyx990803

packages/reactivity/__tests__/collections/Map.spec.ts

@@ -1,7 +1,10 @@
 import { reactive, effect, toRaw, isReactive } from '../../src'
+import { mockWarn } from '@vue/shared'
 
 describe('reactivity/collections', () => {
   describe('Map', () => {
+    mockWarn()
+
     test('instanceof', () => {
       const original = new Map()
       const observed = reactive(original)
@@ -363,6 +366,51 @@ describe('reactivity/collections', () => {
       expect(map.get(key)).toBeUndefined()
     })
 
+    it('should track set of reactive keys in raw map', () => {
+      const raw = new Map()
+      const key = reactive({})
+      raw.set(key, 1)
+      const map = reactive(raw)
+
+      let dummy
+      effect(() => {
+        dummy = map.get(key)
+      })
+      expect(dummy).toBe(1)
+
+      map.set(key, 2)
+      expect(dummy).toBe(2)
+    })
+
+    it('should track deletion of reactive keys in raw map', () => {
+      const raw = new Map()
+      const key = reactive({})
+      raw.set(key, 1)
+      const map = reactive(raw)
+
+      let dummy
+      effect(() => {
+        dummy = map.has(key)
+      })
+      expect(dummy).toBe(true)
+
+      map.delete(key)
+      expect(dummy).toBe(false)
+    })
+
+    it('should warn when both raw and reactive versions of the same object is used as key', () => {
+      const raw = new Map()
+      const rawKey = {}
+      const key = reactive(rawKey)
+      raw.set(rawKey, 1)
+      raw.set(key, 1)
+      const map = reactive(raw)
+      map.set(key, 2)
+      expect(
+        `Reactive Map contains both the raw and reactive`
+      ).toHaveBeenWarned()
+    })
+
     // #877
     it('should not trigger key iteration when setting existing keys', () => {
       const map = reactive(new Map())

packages/reactivity/__tests__/collections/Set.spec.ts

@@ -1,7 +1,10 @@
 import { reactive, effect, isReactive, toRaw } from '../../src'
+import { mockWarn } from '@vue/shared'
 
 describe('reactivity/collections', () => {
   describe('Set', () => {
+    mockWarn()
+
     it('instanceof', () => {
       const original = new Set()
       const observed = reactive(original)
@@ -380,5 +383,34 @@ describe('reactivity/collections', () => {
       expect(set.delete(entry)).toBe(true)
       expect(set.has(entry)).toBe(false)
     })
+
+    it('should track deletion of reactive entries in raw set', () => {
+      const raw = new Set()
+      const entry = reactive({})
+      raw.add(entry)
+      const set = reactive(raw)
+
+      let dummy
+      effect(() => {
+        dummy = set.has(entry)
+      })
+      expect(dummy).toBe(true)
+
+      set.delete(entry)
+      expect(dummy).toBe(false)
+    })
+
+    it('should warn when set contains both raw and reactive versions of the same object', () => {
+      const raw = new Set()
+      const rawKey = {}
+      const key = reactive(rawKey)
+      raw.add(rawKey)
+      raw.add(key)
+      const set = reactive(raw)
+      set.delete(key)
+      expect(
+        `Reactive Set contains both the raw and reactive`
+      ).toHaveBeenWarned()
+    })
   })
 })

packages/reactivity/src/collectionHandlers.ts

@@ -2,7 +2,13 @@ import { toRaw, reactive, readonly } from './reactive'
 import { track, trigger, ITERATE_KEY, MAP_KEY_ITERATE_KEY } from './effect'
 import { TrackOpTypes, TriggerOpTypes } from './operations'
 import { LOCKED } from './lock'
-import { isObject, capitalize, hasOwn, hasChanged } from '@vue/shared'
+import {
+  isObject,
+  capitalize,
+  hasOwn,
+  hasChanged,
+  toRawType
+} from '@vue/shared'
 
 export type CollectionTypes = IterableCollections | WeakCollections
 
@@ -27,6 +33,9 @@ function get(
 ) {
   target = toRaw(target)
   const rawKey = toRaw(key)
+  if (key !== rawKey) {
+    track(target, TrackOpTypes.GET, key)
+  }
   track(target, TrackOpTypes.GET, rawKey)
   const { has, get } = getProto(target)
   if (has.call(target, key)) {
@@ -39,6 +48,9 @@ function get(
 function has(this: CollectionTypes, key: unknown): boolean {
   const target = toRaw(this)
   const rawKey = toRaw(key)
+  if (key !== rawKey) {
+    track(target, TrackOpTypes.HAS, key)
+  }
   track(target, TrackOpTypes.HAS, rawKey)
   const has = getProto(target).has
   return has.call(target, key) || has.call(target, rawKey)
@@ -64,12 +76,19 @@ function add(this: SetTypes, value: unknown) {
 
 function set(this: MapTypes, key: unknown, value: unknown) {
   value = toRaw(value)
-  key = toRaw(key)
   const target = toRaw(this)
-  const proto = getProto(target)
-  const hadKey = proto.has.call(target, key)
-  const oldValue = proto.get.call(target, key)
-  const result = proto.set.call(target, key, value)
+  const { has, get, set } = getProto(target)
+
+  let hadKey = has.call(target, key)
+  if (!hadKey) {
+    key = toRaw(key)
+    hadKey = has.call(target, key)
+  } else if (__DEV__) {
+    checkIdentitiyKeys(target, has, key)
+  }
+
+  const oldValue = get.call(target, key)
+  const result = set.call(target, key, value)
   if (!hadKey) {
     trigger(target, TriggerOpTypes.ADD, key, value)
   } else if (hasChanged(value, oldValue)) {
@@ -85,7 +104,10 @@ function deleteEntry(this: CollectionTypes, key: unknown) {
   if (!hadKey) {
     key = toRaw(key)
     hadKey = has.call(target, key)
+  } else if (__DEV__) {
+    checkIdentitiyKeys(target, has, key)
   }
+
   const oldValue = get ? get.call(target, key) : undefined
   // forward the operation before queueing reactions
   const result = del.call(target, key)
@@ -251,3 +273,21 @@ export const mutableCollectionHandlers: ProxyHandler<CollectionTypes> = {
 export const readonlyCollectionHandlers: ProxyHandler<CollectionTypes> = {
   get: createInstrumentationGetter(readonlyInstrumentations)
 }
+
+function checkIdentitiyKeys(
+  target: CollectionTypes,
+  has: (key: unknown) => boolean,
+  key: unknown
+) {
+  const rawKey = toRaw(key)
+  if (rawKey !== key && has.call(target, rawKey)) {
+    const type = toRawType(target)
+    console.warn(
+      `Reactive ${type} contains both the raw and reactive ` +
+        `versions of the same object${type === `Map` ? `as keys` : ``}, ` +
+        `which can lead to inconsistencies. ` +
+        `Avoid differentiating between the raw and reactive versions ` +
+        `of an object and only use the reactive version if possible.`
+    )
+  }
+}