fix(reactivity): fix shallow readonly behavior for collections (#3003)

fix #3007

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

@@ -0,0 +1,165 @@
+import { isReactive, isReadonly, shallowReadonly } from '../../src'
+
+describe('reactivity/collections', () => {
+  describe('shallowReadonly/Map', () => {
+    ;[Map, WeakMap].forEach(Collection => {
+      test('should make the map/weak-map readonly', () => {
+        const key = {}
+        const val = { foo: 1 }
+        const original = new Collection([[key, val]])
+        const sroMap = shallowReadonly(original)
+        expect(isReadonly(sroMap)).toBe(true)
+        expect(isReactive(sroMap)).toBe(false)
+        expect(sroMap.get(key)).toBe(val)
+
+        sroMap.set(key, {} as any)
+        expect(
+          `Set operation on key "[object Object]" failed: target is readonly.`
+        ).toHaveBeenWarned()
+      })
+
+      test('should not make nested values readonly', () => {
+        const key = {}
+        const val = { foo: 1 }
+        const original = new Collection([[key, val]])
+        const sroMap = shallowReadonly(original)
+        expect(isReadonly(sroMap.get(key))).toBe(false)
+        expect(isReactive(sroMap.get(key))).toBe(false)
+
+        sroMap.get(key)!.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+
+    test('should not make the value generated by the iterable method readonly', () => {
+      const key = {}
+      const val = { foo: 1 }
+      const original = new Map([[key, val]])
+      const sroMap = shallowReadonly(original)
+
+      const values1 = [...sroMap.values()]
+      const values2 = [...sroMap.entries()]
+
+      expect(isReadonly(values1[0])).toBe(false)
+      expect(isReactive(values1[0])).toBe(false)
+      expect(values1[0]).toBe(val)
+
+      values1[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+
+      expect(isReadonly(values2[0][1])).toBe(false)
+      expect(isReactive(values2[0][1])).toBe(false)
+      expect(values2[0][1]).toBe(val)
+
+      values2[0][1].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the forEach method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Map([['key', val]])
+      const sroMap = shallowReadonly(original)
+
+      sroMap.forEach(val => {
+        expect(isReadonly(val)).toBe(false)
+        expect(isReactive(val)).toBe(false)
+        expect(val).toBe(val)
+
+        val.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+  })
+
+  describe('shallowReadonly/Set', () => {
+    test('should make the set/weak-set readonly', () => {
+      ;[Set, WeakSet].forEach(Collection => {
+        const obj = { foo: 1 }
+        const original = new Collection([obj])
+        const sroSet = shallowReadonly(original)
+        expect(isReadonly(sroSet)).toBe(true)
+        expect(isReactive(sroSet)).toBe(false)
+        expect(sroSet.has(obj)).toBe(true)
+
+        sroSet.add({} as any)
+        expect(
+          `Add operation on key "[object Object]" failed: target is readonly.`
+        ).toHaveBeenWarned()
+      })
+    })
+
+    test('should not make nested values readonly', () => {
+      const obj = { foo: 1 }
+      const original = new Set([obj])
+      const sroSet = shallowReadonly(original)
+
+      const values = [...sroSet.values()]
+
+      expect(values[0]).toBe(obj)
+      expect(isReadonly(values[0])).toBe(false)
+      expect(isReactive(values[0])).toBe(false)
+
+      sroSet.add({} as any)
+      expect(
+        `Add operation on key "[object Object]" failed: target is readonly.`
+      ).toHaveBeenWarned()
+
+      values[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the iterable method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Set([val])
+      const sroSet = shallowReadonly(original)
+
+      const values1 = [...sroSet.values()]
+      const values2 = [...sroSet.entries()]
+
+      expect(isReadonly(values1[0])).toBe(false)
+      expect(isReactive(values1[0])).toBe(false)
+      expect(values1[0]).toBe(val)
+
+      values1[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+
+      expect(isReadonly(values2[0][1])).toBe(false)
+      expect(isReactive(values2[0][1])).toBe(false)
+      expect(values2[0][1]).toBe(val)
+
+      values2[0][1].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the forEach method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Set([val])
+      const sroSet = shallowReadonly(original)
+
+      sroSet.forEach(val => {
+        expect(isReadonly(val)).toBe(false)
+        expect(isReactive(val)).toBe(false)
+        expect(val).toBe(val)
+
+        val.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+  })
+})

packages/reactivity/__tests__/readonly.spec.ts

@@ -7,7 +7,6 @@ import {
   markRaw,
   effect,
   ref,
-  shallowReadonly,
   isProxy,
   computed
 } from '../src'
@@ -455,32 +454,4 @@ describe('reactivity/readonly', () => {
       'Set operation on key "randomProperty" failed: target is readonly.'
     ).toHaveBeenWarned()
   })
-
-  describe('shallowReadonly', () => {
-    test('should not make non-reactive properties reactive', () => {
-      const props = shallowReadonly({ n: { foo: 1 } })
-      expect(isReactive(props.n)).toBe(false)
-    })
-
-    test('should make root level properties readonly', () => {
-      const props = shallowReadonly({ n: 1 })
-      // @ts-ignore
-      props.n = 2
-      expect(props.n).toBe(1)
-      expect(
-        `Set operation on key "n" failed: target is readonly.`
-      ).toHaveBeenWarned()
-    })
-
-    // to retain 2.x behavior.
-    test('should NOT make nested properties readonly', () => {
-      const props = shallowReadonly({ n: { foo: 1 } })
-      // @ts-ignore
-      props.n.foo = 2
-      expect(props.n.foo).toBe(2)
-      expect(
-        `Set operation on key "foo" failed: target is readonly.`
-      ).not.toHaveBeenWarned()
-    })
-  })
 })

packages/reactivity/__tests__/shallowReadonly.spec.ts

@@ -0,0 +1,191 @@
+import { isReactive, isReadonly, shallowReadonly } from '../src'
+
+describe('reactivity/shallowReadonly', () => {
+  test('should not make non-reactive properties reactive', () => {
+    const props = shallowReadonly({ n: { foo: 1 } })
+    expect(isReactive(props.n)).toBe(false)
+  })
+
+  test('should make root level properties readonly', () => {
+    const props = shallowReadonly({ n: 1 })
+    // @ts-ignore
+    props.n = 2
+    expect(props.n).toBe(1)
+    expect(
+      `Set operation on key "n" failed: target is readonly.`
+    ).toHaveBeenWarned()
+  })
+
+  // to retain 2.x behavior.
+  test('should NOT make nested properties readonly', () => {
+    const props = shallowReadonly({ n: { foo: 1 } })
+    // @ts-ignore
+    props.n.foo = 2
+    expect(props.n.foo).toBe(2)
+    expect(
+      `Set operation on key "foo" failed: target is readonly.`
+    ).not.toHaveBeenWarned()
+  })
+
+  describe('collection/Map', () => {
+    ;[Map, WeakMap].forEach(Collection => {
+      test('should make the map/weak-map readonly', () => {
+        const key = {}
+        const val = { foo: 1 }
+        const original = new Collection([[key, val]])
+        const sroMap = shallowReadonly(original)
+        expect(isReadonly(sroMap)).toBe(true)
+        expect(isReactive(sroMap)).toBe(false)
+        expect(sroMap.get(key)).toBe(val)
+
+        sroMap.set(key, {} as any)
+        expect(
+          `Set operation on key "[object Object]" failed: target is readonly.`
+        ).toHaveBeenWarned()
+      })
+
+      test('should not make nested values readonly', () => {
+        const key = {}
+        const val = { foo: 1 }
+        const original = new Collection([[key, val]])
+        const sroMap = shallowReadonly(original)
+        expect(isReadonly(sroMap.get(key))).toBe(false)
+        expect(isReactive(sroMap.get(key))).toBe(false)
+
+        sroMap.get(key)!.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+
+    test('should not make the value generated by the iterable method readonly', () => {
+      const key = {}
+      const val = { foo: 1 }
+      const original = new Map([[key, val]])
+      const sroMap = shallowReadonly(original)
+
+      const values1 = [...sroMap.values()]
+      const values2 = [...sroMap.entries()]
+
+      expect(isReadonly(values1[0])).toBe(false)
+      expect(isReactive(values1[0])).toBe(false)
+      expect(values1[0]).toBe(val)
+
+      values1[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+
+      expect(isReadonly(values2[0][1])).toBe(false)
+      expect(isReactive(values2[0][1])).toBe(false)
+      expect(values2[0][1]).toBe(val)
+
+      values2[0][1].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the forEach method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Map([['key', val]])
+      const sroMap = shallowReadonly(original)
+
+      sroMap.forEach(val => {
+        expect(isReadonly(val)).toBe(false)
+        expect(isReactive(val)).toBe(false)
+        expect(val).toBe(val)
+
+        val.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+  })
+
+  describe('collection/Set', () => {
+    test('should make the set/weak-set readonly', () => {
+      ;[Set, WeakSet].forEach(Collection => {
+        const obj = { foo: 1 }
+        const original = new Collection([obj])
+        const sroSet = shallowReadonly(original)
+        expect(isReadonly(sroSet)).toBe(true)
+        expect(isReactive(sroSet)).toBe(false)
+        expect(sroSet.has(obj)).toBe(true)
+
+        sroSet.add({} as any)
+        expect(
+          `Add operation on key "[object Object]" failed: target is readonly.`
+        ).toHaveBeenWarned()
+      })
+    })
+
+    test('should not make nested values readonly', () => {
+      const obj = { foo: 1 }
+      const original = new Set([obj])
+      const sroSet = shallowReadonly(original)
+
+      const values = [...sroSet.values()]
+
+      expect(values[0]).toBe(obj)
+      expect(isReadonly(values[0])).toBe(false)
+      expect(isReactive(values[0])).toBe(false)
+
+      sroSet.add({} as any)
+      expect(
+        `Add operation on key "[object Object]" failed: target is readonly.`
+      ).toHaveBeenWarned()
+
+      values[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the iterable method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Set([val])
+      const sroSet = shallowReadonly(original)
+
+      const values1 = [...sroSet.values()]
+      const values2 = [...sroSet.entries()]
+
+      expect(isReadonly(values1[0])).toBe(false)
+      expect(isReactive(values1[0])).toBe(false)
+      expect(values1[0]).toBe(val)
+
+      values1[0].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+
+      expect(isReadonly(values2[0][1])).toBe(false)
+      expect(isReactive(values2[0][1])).toBe(false)
+      expect(values2[0][1]).toBe(val)
+
+      values2[0][1].foo = 2
+      expect(
+        `Set operation on key "foo" failed: target is readonly.`
+      ).not.toHaveBeenWarned()
+    })
+
+    test('should not make the value generated by the forEach method readonly', () => {
+      const val = { foo: 1 }
+      const original = new Set([val])
+      const sroSet = shallowReadonly(original)
+
+      sroSet.forEach(val => {
+        expect(isReadonly(val)).toBe(false)
+        expect(isReactive(val)).toBe(false)
+        expect(val).toBe(val)
+
+        val.foo = 2
+        expect(
+          `Set operation on key "foo" failed: target is readonly.`
+        ).not.toHaveBeenWarned()
+      })
+    })
+  })
+})

packages/reactivity/src/collectionHandlers.ts

@@ -44,7 +44,7 @@ function get(
   }
   !isReadonly && track(rawTarget, TrackOpTypes.GET, rawKey)
   const { has } = getProto(rawTarget)
-  const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
+  const wrap = isShallow ? toShallow : isReadonly ? toReadonly : toReactive
   if (has.call(rawTarget, key)) {
     return wrap(target.get(key))
   } else if (has.call(rawTarget, rawKey)) {
@@ -151,7 +151,7 @@ function createForEach(isReadonly: boolean, isShallow: boolean) {
     const observed = this as any
     const target = observed[ReactiveFlags.RAW]
     const rawTarget = toRaw(target)
-    const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
+    const wrap = isShallow ? toShallow : isReadonly ? toReadonly : toReactive
     !isReadonly && track(rawTarget, TrackOpTypes.ITERATE, ITERATE_KEY)
     return target.forEach((value: unknown, key: unknown) => {
       // important: make sure the callback is
@@ -191,7 +191,7 @@ function createIterableMethod(
       method === 'entries' || (method === Symbol.iterator && targetIsMap)
     const isKeyOnly = method === 'keys' && targetIsMap
     const innerIterator = target[method](...args)
-    const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
+    const wrap = isShallow ? toShallow : isReadonly ? toReadonly : toReactive
     !isReadonly &&
       track(
         rawTarget,
@@ -279,6 +279,23 @@ const readonlyInstrumentations: Record<string, Function> = {
   forEach: createForEach(true, false)
 }
 
+const shallowReadonlyInstrumentations: Record<string, Function> = {
+  get(this: MapTypes, key: unknown) {
+    return get(this, key, true, true)
+  },
+  get size() {
+    return size((this as unknown) as IterableCollections, true)
+  },
+  has(this: MapTypes, key: unknown) {
+    return has.call(this, key, true)
+  },
+  add: createReadonlyMethod(TriggerOpTypes.ADD),
+  set: createReadonlyMethod(TriggerOpTypes.SET),
+  delete: createReadonlyMethod(TriggerOpTypes.DELETE),
+  clear: createReadonlyMethod(TriggerOpTypes.CLEAR),
+  forEach: createForEach(true, true)
+}
+
 const iteratorMethods = ['keys', 'values', 'entries', Symbol.iterator]
 iteratorMethods.forEach(method => {
   mutableInstrumentations[method as string] = createIterableMethod(
@@ -296,11 +313,18 @@ iteratorMethods.forEach(method => {
     false,
     true
   )
+  shallowReadonlyInstrumentations[method as string] = createIterableMethod(
+    method,
+    true,
+    true
+  )
 })
 
 function createInstrumentationGetter(isReadonly: boolean, shallow: boolean) {
   const instrumentations = shallow
-    ? shallowInstrumentations
+    ? isReadonly
+      ? shallowReadonlyInstrumentations
+      : shallowInstrumentations
     : isReadonly
       ? readonlyInstrumentations
       : mutableInstrumentations
@@ -340,6 +364,12 @@ export const readonlyCollectionHandlers: ProxyHandler<CollectionTypes> = {
   get: createInstrumentationGetter(true, false)
 }
 
+export const shallowReadonlyCollectionHandlers: ProxyHandler<
+  CollectionTypes
+> = {
+  get: createInstrumentationGetter(true, true)
+}
+
 function checkIdentityKeys(
   target: CollectionTypes,
   has: (key: unknown) => boolean,

packages/reactivity/src/reactive.ts

@@ -8,7 +8,8 @@ import {
 import {
   mutableCollectionHandlers,
   readonlyCollectionHandlers,
-  shallowCollectionHandlers
+  shallowCollectionHandlers,
+  shallowReadonlyCollectionHandlers
 } from './collectionHandlers'
 import { UnwrapRef, Ref } from './ref'
 
@@ -159,7 +160,7 @@ export function shallowReadonly<T extends object>(
     target,
     true,
     shallowReadonlyHandlers,
-    readonlyCollectionHandlers
+    shallowReadonlyCollectionHandlers
   )
 }