perf(reactivity): add existing index or non-integer prop on Array sho…

…uld not trigger length dependency (#1969)
vuejs · Aug 26, 2020 · d5c4f6e · d5c4f6e
1 parent 6df0e73
commit d5c4f6e
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 10 deletions.
diff --git a/packages/reactivity/__tests__/reactiveArray.spec.ts b/packages/reactivity/__tests__/reactiveArray.spec.ts
@@ -99,6 +99,33 @@ describe('reactivity/reactive/Array', () => {
     expect(fn).toHaveBeenCalledTimes(1)
   })
 
+  test('add existing index on Array should not trigger length dependency', () => {
+    const array = new Array(3)
+    const observed = reactive(array)
+    const fn = jest.fn()
+    effect(() => {
+      fn(observed.length)
+    })
+    expect(fn).toHaveBeenCalledTimes(1)
+    observed[1] = 1
+    expect(fn).toHaveBeenCalledTimes(1)
+  })
+
+  test('add non-integer prop on Array should not trigger length dependency', () => {
+    const array = new Array(3)
+    const observed = reactive(array)
+    const fn = jest.fn()
+    effect(() => {
+      fn(observed.length)
+    })
+    expect(fn).toHaveBeenCalledTimes(1)
+    // @ts-ignore
+    observed.x = 'x'
+    expect(fn).toHaveBeenCalledTimes(1)
+    observed[-1] = 'x'
+    expect(fn).toHaveBeenCalledTimes(1)
+  })
+
   describe('Array methods w/ refs', () => {
     let original: any[]
     beforeEach(() => {

diff --git a/packages/reactivity/src/baseHandlers.ts b/packages/reactivity/src/baseHandlers.ts
@@ -15,6 +15,7 @@ import {
   isSymbol,
   hasChanged,
   isArray,
+  isIntegerKey,
   extend
 } from '@vue/shared'
 import { isRef } from './ref'
@@ -87,10 +88,7 @@ function createGetter(isReadonly = false, shallow = false) {
 
     if (isRef(res)) {
       // ref unwrapping - does not apply for Array + integer key.
-      const shouldUnwrap =
-        !targetIsArray ||
-        keyIsSymbol ||
-        '' + parseInt(key as string, 10) !== key
+      const shouldUnwrap = !targetIsArray || !isIntegerKey(key)
       return shouldUnwrap ? res.value : res
     }
 
@@ -126,7 +124,10 @@ function createSetter(shallow = false) {
       // in shallow mode, objects are set as-is regardless of reactive or not
     }
 
-    const hadKey = hasOwn(target, key)
+    const hadKey =
+      isArray(target) && isIntegerKey(key)
+        ? Number(key) < target.length
+        : hasOwn(target, key)
     const result = Reflect.set(target, key, value, receiver)
     // don't trigger if target is something up in the prototype chain of original
     if (target === toRaw(receiver)) {

diff --git a/packages/reactivity/src/effect.ts b/packages/reactivity/src/effect.ts
@@ -1,5 +1,5 @@
 import { TrackOpTypes, TriggerOpTypes } from './operations'
-import { EMPTY_OBJ, isArray } from '@vue/shared'
+import { EMPTY_OBJ, isArray, isIntegerKey } from '@vue/shared'
 
 // The main WeakMap that stores {target -> key -> dep} connections.
 // Conceptually, it's easier to think of a dependency as a Dep class
@@ -202,16 +202,17 @@ export function trigger(
       add(depsMap.get(key))
     }
     // also run for iteration key on ADD | DELETE | Map.SET
-    const isAddOrDelete =
-      type === TriggerOpTypes.ADD ||
+    const shouldTriggerIteration =
+      (type === TriggerOpTypes.ADD &&
+        (!isArray(target) || isIntegerKey(key))) ||
       (type === TriggerOpTypes.DELETE && !isArray(target))
     if (
-      isAddOrDelete ||
+      shouldTriggerIteration ||
       (type === TriggerOpTypes.SET && target instanceof Map)
     ) {
       add(depsMap.get(isArray(target) ? 'length' : ITERATE_KEY))
     }
-    if (isAddOrDelete && target instanceof Map) {
+    if (shouldTriggerIteration && target instanceof Map) {
       add(depsMap.get(MAP_KEY_ITERATE_KEY))
     }
   }

diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts
@@ -81,6 +81,9 @@ export const toRawType = (value: unknown): string => {
 export const isPlainObject = (val: unknown): val is object =>
   toTypeString(val) === '[object Object]'
 
+export const isIntegerKey = (key: unknown) =>
+  isString(key) && key[0] !== '-' && '' + parseInt(key, 10) === key
+
 export const isReservedProp = /*#__PURE__*/ makeMap(
   'key,ref,' +
     'onVnodeBeforeMount,onVnodeMounted,' +