Fixed immutable array reducer bug (#1591)

* Fixed immutable array reducer bug

* linting

src/__tests__/actions.spec.js

@@ -1,16 +1,62 @@
 import expect from 'expect'
 import expectPredicate from 'expect-predicate'
 import {
-  ARRAY_INSERT, ARRAY_MOVE, ARRAY_POP, ARRAY_PUSH, ARRAY_REMOVE, ARRAY_REMOVE_ALL, ARRAY_SHIFT,
-  ARRAY_SPLICE, ARRAY_SWAP, ARRAY_UNSHIFT, BLUR, CHANGE, DESTROY, FOCUS, INITIALIZE,
-  REGISTER_FIELD, RESET, SET_SUBMIT_FAILED, SET_SUBMIT_SUCCEEDED,  START_ASYNC_VALIDATION, START_SUBMIT,
-  STOP_ASYNC_VALIDATION, STOP_SUBMIT, TOUCH, UNREGISTER_FIELD, UNTOUCH, UPDATE_SYNC_ERRORS
+  ARRAY_INSERT,
+  ARRAY_MOVE,
+  ARRAY_POP,
+  ARRAY_PUSH,
+  ARRAY_REMOVE,
+  ARRAY_REMOVE_ALL,
+  ARRAY_SHIFT,
+  ARRAY_SPLICE,
+  ARRAY_SWAP,
+  ARRAY_UNSHIFT,
+  BLUR,
+  CHANGE,
+  DESTROY,
+  FOCUS,
+  INITIALIZE,
+  REGISTER_FIELD,
+  RESET,
+  SET_SUBMIT_FAILED,
+  SET_SUBMIT_SUCCEEDED,
+  START_ASYNC_VALIDATION,
+  START_SUBMIT,
+  STOP_ASYNC_VALIDATION,
+  STOP_SUBMIT,
+  TOUCH,
+  UNREGISTER_FIELD,
+  UNTOUCH,
+  UPDATE_SYNC_ERRORS
 } from '../actionTypes'
 import {
-  arrayInsert, arrayMove, arrayPop, arrayPush, arrayRemove, arrayRemoveAll, arrayShift,
-  arraySplice, arraySwap, arrayUnshift, blur, change, destroy, focus, initialize, registerField,
-  reset, setSubmitFailed, setSubmitSucceeded, startAsyncValidation, startSubmit, stopAsyncValidation,
-  stopSubmit, touch, unregisterField, untouch, updateSyncErrors
+  arrayInsert,
+  arrayMove,
+  arrayPop,
+  arrayPush,
+  arrayRemove,
+  arrayRemoveAll,
+  arrayShift,
+  arraySplice,
+  arraySwap,
+  arrayUnshift,
+  blur,
+  change,
+  destroy,
+  focus,
+  initialize,
+  registerField,
+  reset,
+  setSubmitFailed,
+  setSubmitSucceeded,
+  startAsyncValidation,
+  startSubmit,
+  stopAsyncValidation,
+  stopSubmit,
+  touch,
+  unregisterField,
+  untouch,
+  updateSyncErrors
 } from '../actions'
 import { isFSA } from 'flux-standard-action'
 expect.extend(expectPredicate)
@@ -68,6 +114,17 @@ describe('actions', () => {
         payload: 'foo'
       })
       .toPass(isFSA)
+
+    expect(arrayPush('myForm', 'myField'))
+      .toEqual({
+        type: ARRAY_PUSH,
+        meta: {
+          form: 'myForm',
+          field: 'myField'
+        },
+        payload: undefined
+      })
+      .toPass(isFSA)
   })
 
   it('should create array remove action', () => {

src/__tests__/reducer.arrayPush.spec.js

@@ -13,6 +13,18 @@ const describeArrayPush = (reducer, expect, { fromJS }) => () => {
       })
   })
 
+  it('should work pushing undefined to empty state', () => {
+    const state = reducer(undefined, arrayPush('foo', 'myField'))
+    expect(state)
+      .toEqualMap({
+        foo: {
+          values: {
+            myField: [ undefined ]
+          }
+        }
+      })
+  })
+
   it('should work with existing empty array', () => {
     const state = reducer(fromJS({
       foo: {

src/reducer.js

@@ -31,7 +31,17 @@ import 'array-findindex-polyfill'
 import createDeleteInWithCleanUp from './deleteInWithCleanUp'
 
 const createReducer = structure => {
-  const { splice, empty, getIn, setIn, deleteIn, fromJS, size, some, deepEqual } = structure
+  const {
+    deepEqual,
+    empty,
+    getIn,
+    setIn,
+    deleteIn,
+    fromJS,
+    size,
+    some,
+    splice
+  } = structure
   const deleteInWithCleanUp = createDeleteInWithCleanUp(structure)
   const doSplice = (state, key, field, index, removeNum, value, force) => {
     const existing = getIn(state, `${key}.${field}`)
@@ -75,7 +85,7 @@ const createReducer = structure => {
       const length = array ? size(array) : 0
       return length ? arraySplice(state, field, length - 1, 1) : state
     },
-    [ARRAY_PUSH](state, { meta: { field }, payload = empty }) {
+    [ARRAY_PUSH](state, { meta: { field }, payload }) {
       const array = getIn(state, `values.${field}`)
       const length = array ? size(array) : 0
       return arraySplice(state, field, length, 0, payload)

src/structure/immutable/__tests__/splice.spec.js

@@ -3,35 +3,40 @@ import { fromJS, List } from 'immutable'
 import splice from '../splice'
 
 describe('structure.immutable.splice', () => {
-  it('should insert even when initial array is undefined', () => {
-    expect(splice(undefined, 2, 0, 'foo'))  // really goes to index 0
-      .toBeA(List)
-      .toEqual(fromJS([ , , 'foo' ]))  // eslint-disable-line no-sparse-arrays
-  })
+  const testInsertWithValue = value => {
+    it('should insert even when initial array is undefined', () => {
+      expect(splice(undefined, 2, 0, value))  // really goes to index 0
+        .toBeA(List)
+        .toEqual(fromJS([ , , value ]))  // eslint-disable-line no-sparse-arrays
+    })
 
-  it('should insert at start', () => {
-    expect(splice(fromJS([ 'b', 'c', 'd' ]), 0, 0, 'a'))
-      .toBeA(List)
-      .toEqual(fromJS([ 'a', 'b', 'c', 'd' ]))
-  })
+    it('should insert at start', () => {
+      expect(splice(fromJS([ 'b', 'c', 'd' ]), 0, 0, value))
+        .toBeA(List)
+        .toEqual(fromJS([ value, 'b', 'c', 'd' ]))
+    })
 
-  it('should insert at end', () => {
-    expect(splice(fromJS([ 'a', 'b', 'c' ]), 3, 0, 'd'))
-      .toBeA(List)
-      .toEqual(fromJS([ 'a', 'b', 'c', 'd' ]))
-  })
+    it('should insert at end', () => {
+      expect(splice(fromJS([ 'a', 'b', 'c' ]), 3, 0, value))
+        .toBeA(List)
+        .toEqual(fromJS([ 'a', 'b', 'c', value ]))
+    })
 
-  it('should insert in middle', () => {
-    expect(splice(fromJS([ 'a', 'b', 'd' ]), 2, 0, 'c'))
-      .toBeA(List)
-      .toEqual(fromJS([ 'a', 'b', 'c', 'd' ]))
-  })
+    it('should insert in middle', () => {
+      expect(splice(fromJS([ 'a', 'b', 'd' ]), 2, 0, value))
+        .toBeA(List)
+        .toEqual(fromJS([ 'a', 'b', value, 'd' ]))
+    })
 
-  it('should insert in out of range', () => {
-    expect(splice(fromJS([ 'a', 'b', 'c' ]), 5, 0, 'f'))
-      .toBeA(List)
-      .toEqual(fromJS([ 'a', 'b', 'c', , , 'f' ]))  // eslint-disable-line no-sparse-arrays
-  })
+    it('should insert in out of range', () => {
+      expect(splice(fromJS([ 'a', 'b', 'c' ]), 5, 0, value))
+        .toBeA(List)
+        .toEqual(fromJS([ 'a', 'b', 'c', , , value ]))  // eslint-disable-line no-sparse-arrays
+    })
+  }
+
+  testInsertWithValue('value')
+  testInsertWithValue(undefined)
 
   it('should return empty array when removing and initial array is undefined', () => {
     expect(splice(undefined, 2, 1))

src/structure/immutable/expectations.js

@@ -1,6 +1,6 @@
 import expect from 'expect'
 import deepEqual from 'deep-equal'
-import { Map, Iterable, fromJS } from 'immutable'
+import { Map, List, Iterable, fromJS } from 'immutable'
 
 const deepEqualValues = (a, b) => {
   if (Iterable.isIterable(a)) {
@@ -15,7 +15,16 @@ const api = {
   toBeAMap() {
     expect.assert(
       Map.isMap(this.actual),
-      'expected %s to be an immutable map',
+      'expected %s to be an immutable Map',
+      this.actual
+    )
+    return this
+  },
+
+  toBeAList() {
+    expect.assert(
+      List.isList(this.actual),
+      'expected %s to be an immutable List',
       this.actual
     )
     return this

src/structure/immutable/index.js

@@ -6,6 +6,7 @@ import plainGetIn from '../plain/getIn'
 
 const structure = {
   empty: Map(),
+  emptyList: List(),
   getIn: (state, field) =>
     Map.isMap(state) || List.isList(state) ? state.getIn(toPath(field)) : plainGetIn(state, field),
   setIn: (state, field, value) => state.setIn(toPath(field), value),

src/structure/immutable/splice.js

@@ -2,14 +2,19 @@ import { List } from 'immutable'
 
 export default (list = List.isList(list) || List(), index, removeNum, value) => {
   if (index < list.count()) {
+    if (value === undefined && !removeNum) { // inserting undefined
+      // first insert null and then re-set it to undefined
+      return list.splice(index, 0, null).set(index, undefined)
+    }
     if (value != null) {
       return list.splice(index, removeNum, value)  // removing and adding
     } else {
       return list.splice(index, removeNum)  // removing
     }
   }
-  if (value != null) {
-    return list.set(index, value)
+  if (removeNum) { // trying to remove non-existant item: return original array
+    return list
   }
-  return list
+  // trying to add outside of range: just set value
+  return list.set(index, value)
 }

src/structure/plain/__tests__/splice.spec.js

@@ -2,35 +2,40 @@ import expect from 'expect'
 import splice from '../splice'
 
 describe('structure.plain.splice', () => {
-  it('should insert even when initial array is undefined', () => {
-    expect(splice(undefined, 2, 0, 'foo'))  // really goes to index 0
-      .toBeA('array')
-      .toEqual([ , , 'foo' ])  // eslint-disable-line no-sparse-arrays
-  })
+  const testInsertWithValue = value => {
+    it('should insert even when initial array is undefined', () => {
+      expect(splice(undefined, 2, 0, value))  // really goes to index 0
+        .toBeA('array')
+        .toEqual([ , , value ])  // eslint-disable-line no-sparse-arrays
+    })
 
-  it('should insert at start', () => {
-    expect(splice([ 'b', 'c', 'd' ], 0, 0, 'a'))
-      .toBeA('array')
-      .toEqual([ 'a', 'b', 'c', 'd' ])
-  })
+    it(`should insert ${value} at start`, () => {
+      expect(splice([ 'b', 'c', 'd' ], 0, 0, value))
+        .toBeA('array')
+        .toEqual([ value, 'b', 'c', 'd' ])
+    })
 
-  it('should insert at end', () => {
-    expect(splice([ 'a', 'b', 'c' ], 3, 0, 'd'))
-      .toBeA('array')
-      .toEqual([ 'a', 'b', 'c', 'd' ])
-  })
+    it(`should insert ${value} at end`, () => {
+      expect(splice([ 'a', 'b', 'c' ], 3, 0, value))
+        .toBeA('array')
+        .toEqual([ 'a', 'b', 'c', value ])
+    })
 
-  it('should insert in middle', () => {
-    expect(splice([ 'a', 'b', 'd' ], 2, 0, 'c'))
-      .toBeA('array')
-      .toEqual([ 'a', 'b', 'c', 'd' ])
-  })
+    it(`should insert ${value} in middle`, () => {
+      expect(splice([ 'a', 'b', 'd' ], 2, 0, value))
+        .toBeA('array')
+        .toEqual([ 'a', 'b', value, 'd' ])
+    })
 
-  it('should insert in out of range', () => {
-    expect(splice([ 'a', 'b', 'c' ], 5, 0, 'f'))
-      .toBeA('array')
-      .toEqual([ 'a', 'b', 'c', , , 'f' ])  // eslint-disable-line no-sparse-arrays
-  })
+    it(`should insert ${value} when index is out of range`, () => {
+      expect(splice([ 'a', 'b', 'c' ], 5, 0, value))
+        .toBeA('array')
+        .toEqual([ 'a', 'b', 'c', , , value ])  // eslint-disable-line no-sparse-arrays
+    })
+  }
+
+  testInsertWithValue('value')
+  testInsertWithValue(undefined)
 
   it('should return empty array when removing and initial array is undefined', () => {
     expect(splice(undefined, 2, 1))

src/structure/plain/expectations.js

@@ -11,6 +11,15 @@ const expectations = {
     return this
   },
 
+  toBeAList() {
+    expect.assert(
+      Array.isArray(this.actual),
+      'expected %s to be an array',
+      this.actual
+    )
+    return this
+  },
+
   toBeSize(size) {
     expect.assert(
       this.actual && Object.keys(this.actual).length === size,

src/structure/plain/index.js

@@ -7,6 +7,7 @@ import { some } from 'lodash'
 
 const structure = {
   empty: {},
+  emptyList: [],
   getIn,
   setIn,
   deepEqual,

src/structure/plain/splice.js

@@ -1,21 +1,27 @@
 const splice = (array = [], index, removeNum, value) => {
   if (index < array.length) {
-    if (value != null) {
+    if (value === undefined && !removeNum) { // inserting undefined
       const copy = [ ...array ]
-      copy.splice(index, removeNum, value)   // removing and adding
+      copy.splice(index, 0, null)
+      copy[ index ] = undefined
       return copy
-    } else {
+    }
+    if (value != null) {
       const copy = [ ...array ]
-      copy.splice(index, removeNum)   // removing
+      copy.splice(index, removeNum, value)   // removing and adding
       return copy
     }
-  }
-  if (value != null) {
     const copy = [ ...array ]
-    copy[index] = value
+    copy.splice(index, removeNum)   // removing
     return copy
   }
-  return array
+  if (removeNum) { // trying to remove non-existant item: return original array
+    return array
+  }
+  // trying to add outside of range: just set value
+  const copy = [ ...array ]
+  copy[ index ] = value
+  return copy
 }
 
 export default splice