refactor: preserve refs in reactive arrays

BREAKING CHANGE: reactive arrays no longer unwraps contained refs

    When reactive arrays contain refs, especially a mix of refs and
    plain values, Array prototype methods will fail to function
    properly - e.g. sort() or reverse() will overwrite the ref's value
    instead of moving it (see #737).

    Ensuring correct behavior for all possible Array methods while
    retaining the ref unwrapping behavior is exceedinly complicated; In
    addition, even if Vue handles the built-in methods internally, it
    would still break when the user attempts to use a 3rd party utility
    functioon (e.g. lodash) on a reactive array containing refs.

    After this commit, similar to other collection types like Map and
    Set, Arrays will no longer automatically unwrap contained refs.

    The usage of mixed refs and plain values in Arrays should be rare in
    practice. In cases where this is necessary, the user can create a
    computed property that performs the unwrapping.

diff --git a/packages/reactivity/__tests__/reactive.spec.ts b/packages/reactivity/__tests__/reactive.spec.ts
index 1b18dc8055af6894fdea53e8f8010839451d8d58..5d8876843b14c2895fe9bbd4bd44dff02816bda0 100644 (file)
--- a/packages/reactivity/__tests__/reactive.spec.ts
+++ b/packages/reactivity/__tests__/reactive.spec.ts
@@ -26,51 +26,6 @@ describe('reactivity/reactive', () => {
     expect(Object.keys(observed)).toEqual(['foo'])
   })
 
-  test('Array', () => {
-    const original = [{ foo: 1 }]
-    const observed = reactive(original)
-    expect(observed).not.toBe(original)
-    expect(isReactive(observed)).toBe(true)
-    expect(isReactive(original)).toBe(false)
-    expect(isReactive(observed[0])).toBe(true)
-    // get
-    expect(observed[0].foo).toBe(1)
-    // has
-    expect(0 in observed).toBe(true)
-    // ownKeys
-    expect(Object.keys(observed)).toEqual(['0'])
-  })
-
-  test('cloned reactive Array should point to observed values', () => {
-    const original = [{ foo: 1 }]
-    const observed = reactive(original)
-    const clone = observed.slice()
-    expect(isReactive(clone[0])).toBe(true)
-    expect(clone[0]).not.toBe(original[0])
-    expect(clone[0]).toBe(observed[0])
-  })
-
-  test('Array identity methods should work with raw values', () => {
-    const raw = {}
-    const arr = reactive([{}, {}])
-    arr.push(raw)
-    expect(arr.indexOf(raw)).toBe(2)
-    expect(arr.indexOf(raw, 3)).toBe(-1)
-    expect(arr.includes(raw)).toBe(true)
-    expect(arr.includes(raw, 3)).toBe(false)
-    expect(arr.lastIndexOf(raw)).toBe(2)
-    expect(arr.lastIndexOf(raw, 1)).toBe(-1)
-
-    // should work also for the observed version
-    const observed = arr[2]
-    expect(arr.indexOf(observed)).toBe(2)
-    expect(arr.indexOf(observed, 3)).toBe(-1)
-    expect(arr.includes(observed)).toBe(true)
-    expect(arr.includes(observed, 3)).toBe(false)
-    expect(arr.lastIndexOf(observed)).toBe(2)
-    expect(arr.lastIndexOf(observed, 1)).toBe(-1)
-  })
-
   test('nested reactives', () => {
     const original = {
       nested: {
@@ -97,25 +52,6 @@ describe('reactivity/reactive', () => {
     expect('foo' in original).toBe(false)
   })
 
-  test('observed value should proxy mutations to original (Array)', () => {
-    const original: any[] = [{ foo: 1 }, { bar: 2 }]
-    const observed = reactive(original)
-    // set
-    const value = { baz: 3 }
-    const reactiveValue = reactive(value)
-    observed[0] = value
-    expect(observed[0]).toBe(reactiveValue)
-    expect(original[0]).toBe(value)
-    // delete
-    delete observed[0]
-    expect(observed[0]).toBeUndefined()
-    expect(original[0]).toBeUndefined()
-    // mutating methods
-    observed.push(value)
-    expect(observed[2]).toBe(reactiveValue)
-    expect(original[2]).toBe(value)
-  })
-
   test('setting a property with an unobserved value should wrap with reactive', () => {
     const observed = reactive<{ foo?: object }>({})
     const raw = {}

diff --git a/packages/reactivity/__tests__/reactiveArray.spec.ts b/packages/reactivity/__tests__/reactiveArray.spec.ts
new file mode 100644 (file)
index 0000000..b61116e
--- /dev/null
+++ b/packages/reactivity/__tests__/reactiveArray.spec.ts
@@ -0,0 +1,111 @@
+import { reactive, isReactive, toRaw } from '../src/reactive'
+import { ref, isRef } from '../src/ref'
+import { effect } from '../src/effect'
+
+describe('reactivity/reactive/Array', () => {
+  test('should make Array reactive', () => {
+    const original = [{ foo: 1 }]
+    const observed = reactive(original)
+    expect(observed).not.toBe(original)
+    expect(isReactive(observed)).toBe(true)
+    expect(isReactive(original)).toBe(false)
+    expect(isReactive(observed[0])).toBe(true)
+    // get
+    expect(observed[0].foo).toBe(1)
+    // has
+    expect(0 in observed).toBe(true)
+    // ownKeys
+    expect(Object.keys(observed)).toEqual(['0'])
+  })
+
+  test('cloned reactive Array should point to observed values', () => {
+    const original = [{ foo: 1 }]
+    const observed = reactive(original)
+    const clone = observed.slice()
+    expect(isReactive(clone[0])).toBe(true)
+    expect(clone[0]).not.toBe(original[0])
+    expect(clone[0]).toBe(observed[0])
+  })
+
+  test('observed value should proxy mutations to original (Array)', () => {
+    const original: any[] = [{ foo: 1 }, { bar: 2 }]
+    const observed = reactive(original)
+    // set
+    const value = { baz: 3 }
+    const reactiveValue = reactive(value)
+    observed[0] = value
+    expect(observed[0]).toBe(reactiveValue)
+    expect(original[0]).toBe(value)
+    // delete
+    delete observed[0]
+    expect(observed[0]).toBeUndefined()
+    expect(original[0]).toBeUndefined()
+    // mutating methods
+    observed.push(value)
+    expect(observed[2]).toBe(reactiveValue)
+    expect(original[2]).toBe(value)
+  })
+
+  test('Array identity methods should work with raw values', () => {
+    const raw = {}
+    const arr = reactive([{}, {}])
+    arr.push(raw)
+    expect(arr.indexOf(raw)).toBe(2)
+    expect(arr.indexOf(raw, 3)).toBe(-1)
+    expect(arr.includes(raw)).toBe(true)
+    expect(arr.includes(raw, 3)).toBe(false)
+    expect(arr.lastIndexOf(raw)).toBe(2)
+    expect(arr.lastIndexOf(raw, 1)).toBe(-1)
+
+    // should work also for the observed version
+    const observed = arr[2]
+    expect(arr.indexOf(observed)).toBe(2)
+    expect(arr.indexOf(observed, 3)).toBe(-1)
+    expect(arr.includes(observed)).toBe(true)
+    expect(arr.includes(observed, 3)).toBe(false)
+    expect(arr.lastIndexOf(observed)).toBe(2)
+    expect(arr.lastIndexOf(observed, 1)).toBe(-1)
+  })
+
+  test('Array identity methods should be reactive', () => {
+    const obj = {}
+    const arr = reactive([obj, {}])
+
+    let index: number = -1
+    effect(() => {
+      index = arr.indexOf(obj)
+    })
+    expect(index).toBe(0)
+    arr.reverse()
+    expect(index).toBe(1)
+  })
+
+  describe('Array methods w/ refs', () => {
+    let original: any[]
+    beforeEach(() => {
+      original = reactive([1, ref(2)])
+    })
+
+    // read + copy
+    test('read only copy methods', () => {
+      const res = original.concat([3, ref(4)])
+      const raw = toRaw(res)
+      expect(isRef(raw[1])).toBe(true)
+      expect(isRef(raw[3])).toBe(true)
+    })
+
+    // read + write
+    test('read + write mutating methods', () => {
+      const res = original.copyWithin(0, 1, 2)
+      const raw = toRaw(res)
+      expect(isRef(raw[0])).toBe(true)
+      expect(isRef(raw[1])).toBe(true)
+    })
+
+    test('read + indentity', () => {
+      const ref = original[1]
+      expect(ref).toBe(toRaw(original)[1])
+      expect(original.indexOf(ref)).toBe(1)
+    })
+  })
+})

diff --git a/packages/reactivity/__tests__/ref.spec.ts b/packages/reactivity/__tests__/ref.spec.ts
index 84f1c8dff5aa08b0f1fec3abbf8a57f8a0c580ae..dc0cca6c8c3b7bb42bf48d839766c3129e9f6d56 100644 (file)
--- a/packages/reactivity/__tests__/ref.spec.ts
+++ b/packages/reactivity/__tests__/ref.spec.ts
@@ -49,23 +49,20 @@ describe('reactivity/ref', () => {
     const obj = reactive({
       a,
       b: {
-        c: a,
-        d: [a]
+        c: a
       }
     })
 
     let dummy1: number
     let dummy2: number
-    let dummy3: number
 
     effect(() => {
       dummy1 = obj.a
       dummy2 = obj.b.c
-      dummy3 = obj.b.d[0]
     })
 
     const assertDummiesEqualTo = (val: number) =>
-      [dummy1, dummy2, dummy3].forEach(dummy => expect(dummy).toBe(val))
+      [dummy1, dummy2].forEach(dummy => expect(dummy).toBe(val))
 
     assertDummiesEqualTo(1)
     a.value++
@@ -74,8 +71,6 @@ describe('reactivity/ref', () => {
     assertDummiesEqualTo(3)
     obj.b.c++
     assertDummiesEqualTo(4)
-    obj.b.d[0]++
-    assertDummiesEqualTo(5)
   })
 
   it('should unwrap nested ref in types', () => {
@@ -95,15 +90,14 @@ describe('reactivity/ref', () => {
     expect(typeof (c.value.b + 1)).toBe('number')
   })
 
-  it('should properly unwrap ref types nested inside arrays', () => {
+  it('should NOT unwrap ref types nested inside arrays', () => {
     const arr = ref([1, ref(1)]).value
-    // should unwrap to number[]
-    arr[0]++
-    arr[1]++
+    ;(arr[0] as number)++
+    ;(arr[1] as Ref<number>).value++
 
     const arr2 = ref([1, new Map<string, any>(), ref('1')]).value
     const value = arr2[0]
-    if (typeof value === 'string') {
+    if (isRef(value)) {
       value + 'foo'
     } else if (typeof value === 'number') {
       value + 1
@@ -131,8 +125,8 @@ describe('reactivity/ref', () => {
     tupleRef.value[2].a++
     expect(tupleRef.value[2].a).toBe(2)
     expect(tupleRef.value[3]()).toBe(0)
-    tupleRef.value[4]++
-    expect(tupleRef.value[4]).toBe(1)
+    tupleRef.value[4].value++
+    expect(tupleRef.value[4].value).toBe(1)
   })
 
   test('isRef', () => {

diff --git a/packages/reactivity/src/baseHandlers.ts b/packages/reactivity/src/baseHandlers.ts
index 3bc03e0a7f62ce96b0a3751bf34d4fca66b7a97d..11e127d56426684e6502330b7823e56859a46018 100644 (file)
--- a/packages/reactivity/src/baseHandlers.ts
+++ b/packages/reactivity/src/baseHandlers.ts
@@ -16,20 +16,21 @@ const shallowReactiveGet = /*#__PURE__*/ createGetter(false, true)
 const readonlyGet = /*#__PURE__*/ createGetter(true)
 const shallowReadonlyGet = /*#__PURE__*/ createGetter(true, true)
 
-const arrayIdentityInstrumentations: Record<string, Function> = {}
+const arrayInstrumentations: Record<string, Function> = {}
 ;['includes', 'indexOf', 'lastIndexOf'].forEach(key => {
-  arrayIdentityInstrumentations[key] = function(
-    value: unknown,
-    ...args: any[]
-  ): any {
-    return toRaw(this)[key](toRaw(value), ...args)
+  arrayInstrumentations[key] = function(...args: any[]): any {
+    const arr = toRaw(this) as any
+    for (let i = 0, l = (this as any).length; i < l; i++) {
+      track(arr, TrackOpTypes.GET, i + '')
+    }
+    return arr[key](...args.map(toRaw))
   }
 })
 
 function createGetter(isReadonly = false, shallow = false) {
   return function get(target: object, key: string | symbol, receiver: object) {
-    if (isArray(target) && hasOwn(arrayIdentityInstrumentations, key)) {
-      return Reflect.get(arrayIdentityInstrumentations, key, receiver)
+    if (isArray(target) && hasOwn(arrayInstrumentations, key)) {
+      return Reflect.get(arrayInstrumentations, key, receiver)
     }
     const res = Reflect.get(target, key, receiver)
     if (isSymbol(key) && builtInSymbols.has(key)) {
@@ -40,7 +41,8 @@ function createGetter(isReadonly = false, shallow = false) {
       // TODO strict mode that returns a shallow-readonly version of the value
       return res
     }
-    if (isRef(res)) {
+    // ref unwrapping, only for Objects, not for Arrays.
+    if (isRef(res) && !isArray(target)) {
       return res.value
     }
     track(target, TrackOpTypes.GET, key)
@@ -79,7 +81,7 @@ function createSetter(isReadonly = false, shallow = false) {
     const oldValue = (target as any)[key]
     if (!shallow) {
       value = toRaw(value)
-      if (isRef(oldValue) && !isRef(value)) {
+      if (!isArray(target) && isRef(oldValue) && !isRef(value)) {
         oldValue.value = value
         return true
       }
@@ -94,6 +96,7 @@ function createSetter(isReadonly = false, shallow = false) {
       if (!hadKey) {
         trigger(target, TriggerOpTypes.ADD, key, value)
       } else if (hasChanged(value, oldValue)) {
+        debugger
         trigger(target, TriggerOpTypes.SET, key, value, oldValue)
       }
     }

diff --git a/packages/reactivity/src/reactive.ts b/packages/reactivity/src/reactive.ts
index f97a1e6f2d4ef1763b6301862e4a97bd94e5bb8b..5f8aa5797254ad0da1ae5533cdd18e0abdd7984a 100644 (file)
--- a/packages/reactivity/src/reactive.ts
+++ b/packages/reactivity/src/reactive.ts
@@ -9,7 +9,7 @@ import {
   mutableCollectionHandlers,
   readonlyCollectionHandlers
 } from './collectionHandlers'
-import { UnwrapRef, Ref } from './ref'
+import { UnwrapRef, Ref, isRef } from './ref'
 import { makeMap } from '@vue/shared'
 
 // WeakMaps that store {raw <-> observed} pairs.
@@ -50,6 +50,9 @@ export function reactive(target: object) {
   if (readonlyValues.has(target)) {
     return readonly(target)
   }
+  if (isRef(target)) {
+    return target
+  }
   return createReactiveObject(
     target,
     rawToReactive,

diff --git a/packages/reactivity/src/ref.ts b/packages/reactivity/src/ref.ts
index b36b3308b3946d7805426da06ac31cf3898b65b0..351dd1ddb5d5c55539dcd95df0561e58a370fd80 100644 (file)
--- a/packages/reactivity/src/ref.ts
+++ b/packages/reactivity/src/ref.ts
@@ -29,7 +29,6 @@ export function isRef(r: any): r is Ref {
 }
 
 export function ref<T>(value: T): T extends Ref ? T : Ref<T>
-export function ref<T>(value: T): Ref<T>
 export function ref<T = any>(): Ref<T>
 export function ref(value?: unknown) {
   if (isRef(value)) {
@@ -83,8 +82,6 @@ function toProxyRef<T extends object, K extends keyof T>(
   } as any
 }
 
-type UnwrapArray<T> = { [P in keyof T]: UnwrapRef<T[P]> }
-
 // corner case when use narrows type
 // Ex. type RelativePath = string & { __brand: unknown }
 // RelativePath extends object -> true
@@ -94,7 +91,7 @@ type BaseTypes = string | number | boolean
 export type UnwrapRef<T> = {
   cRef: T extends ComputedRef<infer V> ? UnwrapRef<V> : T
   ref: T extends Ref<infer V> ? UnwrapRef<V> : T
-  array: T extends Array<infer V> ? Array<UnwrapRef<V>> & UnwrapArray<T> : T
+  array: T
   object: { [K in keyof T]: UnwrapRef<T[K]> }
 }[T extends ComputedRef<any>
   ? 'cRef'