Skip to content

Commit

Permalink
refactor: preserve refs in reactive arrays
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yyx990803 committed Feb 21, 2020
1 parent 627b9df commit 775a7c2
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 93 deletions.
64 changes: 0 additions & 64 deletions packages/reactivity/__tests__/reactive.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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 = {}
Expand Down
111 changes: 111 additions & 0 deletions packages/reactivity/__tests__/reactiveArray.spec.ts
Original file line number Diff line number Diff line change
@@ -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)
})
})
})
22 changes: 8 additions & 14 deletions packages/reactivity/__tests__/ref.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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++
Expand All @@ -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', () => {
Expand All @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down
23 changes: 13 additions & 10 deletions packages/reactivity/src/baseHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
}
}
Expand Down
5 changes: 4 additions & 1 deletion packages/reactivity/src/reactive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 1 addition & 4 deletions packages/reactivity/src/ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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
Expand All @@ -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'
Expand Down

0 comments on commit 775a7c2

Please sign in to comment.