Fix removeBatch mutator removing more fields than expected.

Author: huan086

src/removeBatch.js

@@ -1,62 +1,97 @@
 // @flow
 import type { MutableState, Mutator, Tools } from 'final-form'
-import moveFieldState from './moveFieldState'
+import copyField from './copyField'
 import { escapeRegexTokens } from './utils'
 
-const countBelow = (array, value) =>
-  array.reduce((count, item) => (item < value ? count + 1 : count), 0)
+const binarySearch = (list: number[], value: number): number => {
+  // This algorithm assumes the items in list is unique
+  let first = 0
+  let last = list.length - 1
+  let middle = 0
+
+  while (first <= last) {
+    middle = Math.floor((first + last) / 2)
+    if (list[middle] === value) {
+      return middle
+    }
+
+    if (list[middle] > value) {
+      last = middle - 1
+    } else {
+      first = middle + 1
+    }
+  }
+
+  return ~first
+}
 
 const removeBatch: Mutator<any> = (
   [name, indexes]: any[],
   state: MutableState<any>,
   { changeValue }: Tools<any>
 ) => {
+  if (indexes.length === 0) {
+    return []
+  }
+
   const sortedIndexes: number[] = [...indexes]
   sortedIndexes.sort()
-  // remove duplicates
-  for (let i = 0; i < sortedIndexes.length; i++) {
-    if (i > 0 && sortedIndexes[i] === sortedIndexes[i - 1]) {
-      sortedIndexes.splice(i--, 1)
+
+  // Remove duplicates
+  for (let i = sortedIndexes.length - 1; i > 0; i -= 1) {
+    if (sortedIndexes[i] === sortedIndexes[i - 1]) {
+      sortedIndexes.splice(i, 1)
     }
   }
 
   let returnValue = []
   changeValue(state, name, (array: ?(any[])): ?(any[]) => {
     // use original order of indexes for return value
     returnValue = indexes.map(index => array && array[index])
-    if (!array || !sortedIndexes.length) {
+
+    if (!array) {
       return array
     }
 
     const copy = [...array]
-    const removed = []
-    sortedIndexes.forEach((index: number) => {
-      copy.splice(index - removed.length, 1)
-      removed.push(array && array[index])
-    })
+    for (let i = sortedIndexes.length - 1; i >= 0; i -= 1) {
+      const index = sortedIndexes[i]
+      copy.splice(index, 1)
+    }
+
     return copy
   })
 
   // now we have to remove any subfields for our indexes,
   // and decrement all higher indexes.
   const pattern = new RegExp(`^${escapeRegexTokens(name)}\\[(\\d+)\\](.*)`)
-  const newState = { ...state, fields: {} }
+  const newFields = {}
   Object.keys(state.fields).forEach(key => {
     const tokens = pattern.exec(key)
     if (tokens) {
       const fieldIndex = Number(tokens[1])
-      if (!~sortedIndexes.indexOf(fieldIndex)) {
-        // not one of the removed indexes
-        // shift all higher ones down
-        const decrementedKey = `${name}[${fieldIndex -
-          countBelow(sortedIndexes, fieldIndex)}]${tokens[2]}`
-        moveFieldState(newState, state.fields[key], decrementedKey, state)
+      const indexOfFieldIndex = binarySearch(sortedIndexes, fieldIndex)
+      if (indexOfFieldIndex >= 0) {
+        // One of the removed indices
+        return
+      }
+
+      if (fieldIndex > sortedIndexes[0]) {
+        // Shift all higher indices down
+        const decrementedKey = `${name}[${fieldIndex - ~indexOfFieldIndex}]${
+          tokens[2]
+        }`
+        copyField(state.fields, key, newFields, decrementedKey)
+        return
       }
-    } else {
-      newState.fields[key] = state.fields[key]
     }
+
+    // Keep this field that does not match the name,
+    // or has index smaller than what is being removed
+    newFields[key] = state.fields[key]
   })
-  state.fields = newState.fields
+
+  state.fields = newFields
   return returnValue
 }
 

src/removeBatch.test.js

@@ -187,11 +187,51 @@ describe('removeBatch', () => {
     expect(result).toBeUndefined()
   })
 
-  it('should return the original array if no indexes are specified to be removed', () => {
-    const op = getOp([])
-    const result = op(['a', 'b', 'c', 'd', 'e'])
-    expect(Array.isArray(result)).toBe(true)
-    expect(result).toEqual(['a', 'b', 'c', 'd', 'e'])
+  it('should keep the original state if no indexes are specified to be removed', () => {
+    const array = ['a', 'b', 'c', 'd', 'e']
+    function blur0() {}
+    function change0() {}
+    function focus0() {}
+    const state = {
+      formState: {
+        values: {
+          foo: array
+        }
+      },
+      fields: {
+        'foo[0]': {
+          name: 'foo[0]',
+          blur: blur0,
+          change: change0,
+          focus: focus0,
+          touched: true,
+          error: 'A Error'
+        }
+      }
+    }
+    const changeValue = jest.fn()
+    const returnValue = removeBatch(['foo[0]', []], state, {
+      changeValue
+    })
+    expect(returnValue).toEqual([])
+    expect(state.formState.values.foo).toBe(array) // no change
+    expect(state).toEqual({
+      formState: {
+        values: {
+          foo: array
+        }
+      },
+      fields: {
+        'foo[0]': {
+          name: 'foo[0]',
+          blur: blur0,
+          change: change0,
+          focus: focus0,
+          touched: true,
+          error: 'A Error'
+        }
+      }
+    })
   })
 
   it('should remove the values at the specified indexes', () => {
@@ -239,6 +279,14 @@ describe('removeBatch', () => {
         }
       },
       fields: {
+        'foo[4]': {
+          name: 'foo[4]',
+          blur: blur4,
+          change: change4,
+          focus: focus4,
+          touched: true,
+          error: 'E Error'
+        },
         'foo[0]': {
           name: 'foo[0]',
           blur: blur0,
@@ -271,31 +319,32 @@ describe('removeBatch', () => {
           touched: false,
           error: 'D Error'
         },
-        'foo[4]': {
-          name: 'foo[4]',
-          blur: blur4,
-          change: change4,
-          focus: focus4,
-          touched: true,
-          error: 'E Error'
-        },
         anotherField: {
           name: 'anotherField',
           touched: false
         }
       }
     }
-    const returnValue = removeBatch(['foo', [1, 2]], state, { changeValue })
-    expect(returnValue).toEqual(['b', 'c'])
+    const returnValue = removeBatch(['foo', [1, 3]], state, { changeValue })
+    expect(returnValue).toEqual(['b', 'd'])
     expect(state.formState.values.foo).not.toBe(array) // copied
     expect(state).toEqual({
       formState: {
         values: {
-          foo: ['a', 'd', 'e'],
+          foo: ['a', 'c', 'e'],
           anotherField: 42
         }
       },
       fields: {
+        'foo[2]': {
+          name: 'foo[2]',
+          blur: blur2,
+          change: change2,
+          focus: focus2,
+          touched: true,
+          error: 'E Error',
+          lastFieldState: undefined
+        },
         'foo[0]': {
           name: 'foo[0]',
           blur: blur0,
@@ -310,17 +359,8 @@ describe('removeBatch', () => {
           blur: blur1,
           change: change1,
           focus: focus1,
-          touched: false,
-          error: 'D Error',
-          lastFieldState: undefined
-        },
-        'foo[2]': {
-          name: 'foo[2]',
-          blur: blur2,
-          change: change2,
-          focus: focus2,
           touched: true,
-          error: 'E Error',
+          error: 'C Error',
           lastFieldState: undefined
         },
         anotherField: {
@@ -362,6 +402,14 @@ describe('removeBatch', () => {
         }
       },
       fields: {
+        'foo[0][4]': {
+          name: 'foo[0][4]',
+          blur: blur4,
+          change: change4,
+          focus: focus4,
+          touched: true,
+          error: 'E Error'
+        },
         'foo[0][0]': {
           name: 'foo[0][0]',
           blur: blur0,
@@ -394,33 +442,34 @@ describe('removeBatch', () => {
           touched: false,
           error: 'D Error'
         },
-        'foo[0][4]': {
-          name: 'foo[0][4]',
-          blur: blur4,
-          change: change4,
-          focus: focus4,
-          touched: true,
-          error: 'E Error'
-        },
         anotherField: {
           name: 'anotherField',
           touched: false
         }
       }
     }
-    const returnValue = removeBatch(['foo[0]', [1, 2]], state, {
+    const returnValue = removeBatch(['foo[0]', [1, 3]], state, {
       changeValue
     })
-    expect(returnValue).toEqual(['b', 'c'])
+    expect(returnValue).toEqual(['b', 'd'])
     expect(state.formState.values.foo).not.toBe(array) // copied
     expect(state).toEqual({
       formState: {
         values: {
-          foo: [['a', 'd', 'e']],
+          foo: [['a', 'c', 'e']],
           anotherField: 42
         }
       },
       fields: {
+        'foo[0][2]': {
+          name: 'foo[0][2]',
+          blur: blur2,
+          change: change2,
+          focus: focus2,
+          touched: true,
+          error: 'E Error',
+          lastFieldState: undefined
+        },
         'foo[0][0]': {
           name: 'foo[0][0]',
           blur: blur0,
@@ -435,17 +484,8 @@ describe('removeBatch', () => {
           blur: blur1,
           change: change1,
           focus: focus1,
-          touched: false,
-          error: 'D Error',
-          lastFieldState: undefined
-        },
-        'foo[0][2]': {
-          name: 'foo[0][2]',
-          blur: blur2,
-          change: change2,
-          focus: focus2,
           touched: true,
-          error: 'E Error',
+          error: 'C Error',
           lastFieldState: undefined
         },
         anotherField: {