Repository: erikras/redux-form
Pull request #647 (2016-02-15 18:44:42)

Normalizing array fields
This PR fixes several bugs related to deep field mutations and normalization.

@jfairbank I apologize for [misunderstanding you before](https://github.com/erikras/redux-form/commit/dc581172784686af13754c0d07cb9f502eb9fde2#commitcomment-16075774) (I was on a layover between flights and not thinking straight). You were right. The array values need to be wrapped with `{value: }`.


Patch:
diff --git a/src/getValues.js b/src/getValues.js
index 970c9003..0b8a5fbc 100644
--- a/src/getValues.js
+++ b/src/getValues.js
@@ -24,7 +24,7 @@ const getValue = (field, state, dest) => {
         getValue(rest, item, dest[key][index]);
       });
     } else {
-      dest[key] = array.map(item => item.value);
+      dest[key] = array.map(item => item && item.value);
     }
   } else if (dotIndex > 0) {
     // subobject field
diff --git a/src/normalizeFields.js b/src/normalizeFields.js
index 56e331e9..f448765c 100644
--- a/src/normalizeFields.js
+++ b/src/normalizeFields.js
@@ -54,12 +54,13 @@ function normalizeField(field, fullFieldPath, state, previousState, values, prev
 
     const normalizer = normalizers[fullFieldPath];
 
-    return normalizer(
+    const result = normalizer(
       state && state[field.key],
       previousState && previousState[field.key],
       values,
       previousValues
     );
+    return field.isArray ? result && result.map(makeFieldValue) : result;
   } else if (field.nestedPath) {
     const nestedState = state && state[field.key] || {};
     const nestedField = extractKey(field.nestedPath);
diff --git a/src/readField.js b/src/readField.js
index 24951e61..5cd8467f 100644
--- a/src/readField.js
+++ b/src/readField.js
@@ -15,6 +15,15 @@ function getSuffix(input, closeIndex) {
   return suffix;
 }
 
+const getNextKey = path => {
+  const dotIndex = path.indexOf('.');
+  const openIndex = path.indexOf('[');
+  if (openIndex > 0 && (dotIndex < 0 || openIndex < dotIndex)) {
+    return path.substring(0, openIndex);
+  }
+  return dotIndex > 0 ? path.substring(0, dotIndex) : path;
+};
+
 const readField = (state, fieldName, pathToHere = '', fields, syncErrors, asyncValidate, isReactNative, props, callback = () => null, prefix = '') => {
   const {asyncBlurFields, blur, change, focus, form, initialValues, readonly, addArrayValue,
     removeArrayValue, swapArrayValues} = props;
@@ -39,22 +48,28 @@ const readField = (state, fieldName, pathToHere = '', fields, syncErrors, asyncV
         return accumulator;
       }, [])
       .map(field => getSuffix(field, prefix.length + closeIndex));
-    if (!fields[key] || fields[key].length !== stateArray.length) {
-      fields[key] = fields[key] ? [...fields[key]] : [];
-      Object.defineProperty(fields[key], 'addField', {
+    const addMethods = dest => {
+      Object.defineProperty(dest, 'addField', {
         value: (value, index) => addArrayValue(pathToHere + key, value, index, subfields)
       });
-      Object.defineProperty(fields[key], 'removeField', {
+      Object.defineProperty(dest, 'removeField', {
         value: index => removeArrayValue(pathToHere + key, index)
       });
-      Object.defineProperty(fields[key], 'swapFields', {
+      Object.defineProperty(dest, 'swapFields', {
         value: (indexA, indexB) => swapArrayValues(pathToHere + key, indexA, indexB)
       });
+      return dest;
+    };
+    if (!fields[key] || fields[key].length !== stateArray.length) {
+      fields[key] = fields[key] ? [...fields[key]] : [];
+      addMethods(fields[key]);
     }
     const fieldArray = fields[key];
+    let changed = false;
     stateArray.forEach((fieldState, index) => {
       if (rest && !fieldArray[index]) {
         fieldArray[index] = {};
+        changed = true;
       }
       const dest = rest ? fieldArray[index] : {};
       const nextPath = `${pathToHere}${key}[${index}]${rest ? '.' : ''}`;
@@ -62,34 +77,36 @@ const readField = (state, fieldName, pathToHere = '', fields, syncErrors, asyncV
 
       const result = readField(fieldState, rest, nextPath, dest, syncErrors,
         asyncValidate, isReactNative, props, callback, nextPrefix);
-      if (!rest) { // if nothing after [] in field name, assign directly to array
+      if (!rest && fieldArray[index] !== result) {
+        // if nothing after [] in field name, assign directly to array
         fieldArray[index] = result;
+        changed = true;
       }
     });
     if (fieldArray.length > stateArray.length) {
       // remove extra items that aren't in state array
       fieldArray.splice(stateArray.length, fieldArray.length - stateArray.length);
     }
-    return fieldArray;
+    return changed ? addMethods([...fieldArray]) : fieldArray;
   }
   if (dotIndex > 0) {
     // subobject field
     const key = fieldName.substring(0, dotIndex);
     const rest = fieldName.substring(dotIndex + 1);
-    if (!fields[key]) {
-      fields[key] = {};
-    }
+    let subobject = fields[key] || {};
     const nextPath = pathToHere + key + '.';
-    const previous = fields[key];
-    const result = readField(state[key] || {}, rest, nextPath, fields[key], syncErrors, asyncValidate,
+    const nextKey = getNextKey(rest);
+    const previous = subobject[nextKey];
+    const result = readField(state[key] || {}, rest, nextPath, subobject, syncErrors, asyncValidate,
       isReactNative, props, callback, nextPath);
     if (result !== previous) {
-      fields[key] = {
-        ...fields[key],
-        [key]: result
+      subobject = {
+        ...subobject,
+        [nextKey]: result
       };
     }
-    return result;
+    fields[key] = subobject;
+    return subobject;
   }
   const name = pathToHere + fieldName;
   const field = fields[fieldName] || {};


Test patch:
diff --git a/src/__tests__/createReduxForm.spec.js b/src/__tests__/createReduxForm.spec.js
index e63693fb..1638c235 100644
--- a/src/__tests__/createReduxForm.spec.js
+++ b/src/__tests__/createReduxForm.spec.js
@@ -1947,7 +1947,7 @@ describe('createReduxForm', () => {
     expect(stub.props.fields.curly).toBe(curly);
   });
 
-  it('should only mutate the deep field that changed', () => {
+  it('should only change the deep field that changed', () => {
     const store = makeStore();
     const form = 'testForm';
     const Decorated = reduxForm({
@@ -1972,6 +1972,49 @@ describe('createReduxForm', () => {
     expect(stub.props.fields.address.postalCode).toNotBe(postalCode);
   });
 
+  it('should change field tree up to array that changed', () => {
+    const store = makeStore();
+    const form = 'testForm';
+    const Decorated = reduxForm({
+      form,
+      fields: ['contact.shipping.phones[]', 'contact.billing.phones[]']
+    })(Form);
+    const dom = TestUtils.renderIntoDocument(
+      <Provider store={store}>
+        <Decorated/>
+      </Provider>
+    );
+    const stub = TestUtils.findRenderedComponentWithType(dom, Form);
+
+    let contact = stub.props.fields.contact;
+    let shipping = stub.props.fields.contact.shipping;
+    let shippingPhones = stub.props.fields.contact.shipping.phones;
+    const billing = stub.props.fields.contact.billing;
+    const billingPhones = stub.props.fields.contact.billing.phones;
+
+    shippingPhones.addField();
+
+    expect(stub.props.fields.contact.shipping.phones).toNotBe(shippingPhones);
+    expect(stub.props.fields.contact.shipping).toNotBe(shipping);
+    expect(stub.props.fields.contact).toNotBe(contact);
+    expect(stub.props.fields.contact.billing).toBe(billing);
+    expect(stub.props.fields.contact.billing.phones).toBe(billingPhones);
+
+    contact = stub.props.fields.contact;
+    shipping = stub.props.fields.contact.shipping;
+    shippingPhones = stub.props.fields.contact.shipping.phones;
+    const shippingPhones0 = stub.props.fields.contact.shipping.phones[0];
+
+    shippingPhones[0].onChange('555-1234');
+
+    expect(stub.props.fields.contact.shipping.phones[0]).toNotBe(shippingPhones0);
+    expect(stub.props.fields.contact.shipping.phones).toNotBe(shippingPhones);
+    expect(stub.props.fields.contact.shipping).toNotBe(shipping);
+    expect(stub.props.fields.contact).toNotBe(contact);
+    expect(stub.props.fields.contact.billing).toBe(billing);
+    expect(stub.props.fields.contact.billing.phones).toBe(billingPhones);
+  });
+
   it('should only rerender the form that changed', () => {
     const store = makeStore();
     const fooRender = createRestorableSpy().andReturn(<div/>);
diff --git a/src/__tests__/reducer.spec.js b/src/__tests__/reducer.spec.js
index e47949f5..76c898b2 100644
--- a/src/__tests__/reducer.spec.js
+++ b/src/__tests__/reducer.spec.js
@@ -5,6 +5,16 @@ import {addArrayValue, blur, change, focus, initialize, removeArrayValue, reset,
   stopAsyncValidation, stopSubmit, swapArrayValues, touch, untouch, destroy} from '../actions';
 import {isFieldValue, makeFieldValue} from '../fieldValue';
 
+const compare = (a, b) => {
+  if (a.value > b.value) {
+    return 1;
+  }
+  if (a.value < b.value) {
+    return -1;
+  }
+  return 0;
+};
+
 describe('reducer', () => {
   it('should initialize state to {}', () => {
     const state = reducer();
@@ -360,7 +370,10 @@ describe('reducer', () => {
         _submitFailed: false
       }
     }, {
-      ...addArrayValue('myField', {foo: {a: 'foo-a3', b: 'foo-b3'}, bar: {a: 'bar-a3', b: 'bar-b3'}}, undefined),
+      ...addArrayValue('myField', {
+        foo: {a: 'foo-a3', b: 'foo-b3'},
+        bar: {a: 'bar-a3', b: 'bar-b3'}
+      }, undefined),
       form: 'testForm'
     });
     expect(state.testForm)
@@ -3081,8 +3094,8 @@ describe('reducer', () => {
             }
           },
           pets: [
-            { name: { value: 'Fido' } },
-            { name: { value: 'Tucker' } }
+            {name: {value: 'Fido'}},
+            {name: {value: 'Tucker'}}
           ]
         }
       });
@@ -3103,8 +3116,8 @@ describe('reducer', () => {
             }
           },
           pets: [
-            { name: { value: 'fido' } },
-            { name: { value: 'tucker' } }
+            {name: {value: 'fido'}},
+            {name: {value: 'tucker'}}
           ]
         });
     });
@@ -3136,7 +3149,7 @@ describe('reducer', () => {
       });
       state = normalizingReducer(state, {
         form: 'foo',
-        ...addArrayValue('pets', { name: 'Fido' })
+        ...addArrayValue('pets', {name: 'Fido'})
       });
       expect(state)
         .toExist()
@@ -3204,43 +3217,64 @@ describe('reducer', () => {
           'name': () => 'normalized',
           'person.name': (name) => name && name.toUpperCase(),
           'pets[].name': (name) => name && name.toLowerCase(),
-          'cats[]': (array) => array && array.map(value => value.toUpperCase()),
-          'programming[].langs[]': (array) => array && array.slice(0).sort(),
-          'some.numbers[]': (array) => array && array.filter(n => n % 2 === 0),
+          'cats[]': (array) => array && array.map(({value}) => ({value: value.toUpperCase()})),
+          'programming[].langs[]': (array) => array && array.slice(0).sort(compare),
+          'some.numbers[]': (array) => array && array.filter(({value}) => value % 2 === 0),
           'a.very.deep.object.property': (value) => value && value.toUpperCase(),
-          'my[].deeply[].nested.arrayItem': (value) => value && value.toUpperCase()
+          'my[].deeply[].nested.item': (value) => value && value.toUpperCase()
         }
       });
       const state = normalize({
         foo: {
           person: {
-            name: {
-              value: 'John Doe',
-            }
+            name: makeFieldValue({value: 'John Doe'})
           },
           pets: [
-            { name: { value: 'Fido' } },
-            { name: { value: 'Tucker' } }
+            {name: makeFieldValue({value: 'Fido'})},
+            {name: makeFieldValue({value: 'Tucker'})}
           ],
           cats: [
-            'lion',
-            'panther',
-            'garfield',
-            'whiskers'
+            makeFieldValue({value: 'lion'}),
+            makeFieldValue({value: 'panther'}),
+            makeFieldValue({value: 'garfield'}),
+            makeFieldValue({value: 'whiskers'})
           ],
           programming: [{
-            langs: ['ml', 'ocaml', 'lisp', 'haskell', 'f#']
+            langs: [
+              makeFieldValue({value: 'ml'}),
+              makeFieldValue({value: 'ocaml'}),
+              makeFieldValue({value: 'lisp'}),
+              makeFieldValue({value: 'haskell'}),
+              makeFieldValue({value: 'f#'})
+            ]
           }, {
-            langs: ['smalltalk', 'ruby', 'java', 'c#', 'c++']
+            langs: [
+              makeFieldValue({value: 'smalltalk'}),
+              makeFieldValue({value: 'ruby'}),
+              makeFieldValue({value: 'java'}),
+              makeFieldValue({value: 'c#'}),
+              makeFieldValue({value: 'c++'})
+            ]
           }],
           some: {
-            numbers: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+            numbers: [
+              makeFieldValue({value: 1}),
+              makeFieldValue({value: 2}),
+              makeFieldValue({value: 3}),
+              makeFieldValue({value: 4}),
+              makeFieldValue({value: 5}),
+              makeFieldValue({value: 6}),
+              makeFieldValue({value: 7}),
+              makeFieldValue({value: 8}),
+              makeFieldValue({value: 9}),
+              makeFieldValue({value: 10})
+            ]
           },
           a: {
             very: {
               deep: {
                 object: {
-                  property: { value: 'test' }
+                  property: makeFieldValue({value: 'test'})
                 }
               }
             }
@@ -3248,33 +3282,33 @@ describe('reducer', () => {
           my: [{
             deeply: [{
               nested: {
-                arrayItem: { value: 'hello' },
-                not: 'lost'
+                item: makeFieldValue({value: 'hello'}),
+                not: makeFieldValue({value: 'lost'})
               },
-              otherKey: { value: 'Goodbye' }
+              otherKey: makeFieldValue({value: 'Goodbye'})
             }, {
               nested: {
-                arrayItem: { value: 'hola' },
-                not: 'lost'
+                item: makeFieldValue({value: 'hola'}),
+                not: makeFieldValue({value: 'lost'})
               },
-              otherKey: { value: 'Adios' }
+              otherKey: makeFieldValue({value: 'Adios'})
             }],
-            stays: 'intact'
+            stays: makeFieldValue({value: 'intact'})
           }, {
             deeply: [{
               nested: {
-                arrayItem: { value: 'world' },
-                not: 'lost'
+                item: makeFieldValue({value: 'world'}),
+                not: makeFieldValue({value: 'lost'})
               },
-              otherKey: { value: 'Later' }
+              otherKey: makeFieldValue({value: 'Later'})
             }, {
               nested: {
-                arrayItem: { value: 'mundo' },
-                not: 'lost'
+                item: makeFieldValue({value: 'mundo'}),
+                not: makeFieldValue({value: 'lost'})
               },
-              otherKey: { value: 'Hasta luego' }
+              otherKey: makeFieldValue({value: 'Hasta luego'})
             }],
-            stays: 'intact'
+            stays: makeFieldValue({value: 'intact'})
           }]
         }
       });
@@ -3286,37 +3320,51 @@ describe('reducer', () => {
         .toBeA('object')
         .toEqual({
           ...defaultFields,
-          name: {
-            value: 'normalized'
-          },
+          name: {value: 'normalized'},
           person: {
-            name: {
-              value: 'JOHN DOE'
-            }
+            name: {value: 'JOHN DOE'}
           },
           pets: [
-            { name: { value: 'fido' } },
-            { name: { value: 'tucker' } }
+            {name: {value: 'fido'}},
+            {name: {value: 'tucker'}}
           ],
           cats: [
-            'LION',
-            'PANTHER',
-            'GARFIELD',
-            'WHISKERS'
+            {value: 'LION'},
+            {value: 'PANTHER'},
+            {value: 'GARFIELD'},
+            {value: 'WHISKERS'}
           ],
           programming: [{
-            langs: ['f#', 'haskell', 'lisp', 'ml', 'ocaml']
+            langs: [
+              {value: 'f#'},
+              {value: 'haskell'},
+              {value: 'lisp'},
+              {value: 'ml'},
+              {value: 'ocaml'}
+            ]
           }, {
-            langs: ['c#', 'c++', 'java', 'ruby', 'smalltalk']
+            langs: [
+              {value: 'c#'},
+              {value: 'c++'},
+              {value: 'java'},
+              {value: 'ruby'},
+              {value: 'smalltalk'}
+            ]
           }],
           some: {
-            numbers: [2, 4, 6, 8, 10]
+            numbers: [
+              {value: 2},
+              {value: 4},
+              {value: 6},
+              {value: 8},
+              {value: 10}
+            ]
           },
           a: {
             very: {
               deep: {
                 object: {
-                  property: { value: 'TEST' }
+                  property: {value: 'TEST'}
                 }
               }
             }
@@ -3324,35 +3372,73 @@ describe('reducer', () => {
           my: [{
             deeply: [{
               nested: {
-                arrayItem: { value: 'HELLO' },
-                not: 'lost'
+                item: {value: 'HELLO'},
+                not: {value: 'lost'}
               },
-              otherKey: { value: 'Goodbye' }
+              otherKey: {value: 'Goodbye'}
             }, {
               nested: {
-                arrayItem: { value: 'HOLA' },
-                not: 'lost'
+                item: {value: 'HOLA'},
+                not: {value: 'lost'}
               },
-              otherKey: { value: 'Adios' }
+              otherKey: {value: 'Adios'}
             }],
-            stays: 'intact'
+            stays: {value: 'intact'}
           }, {
             deeply: [{
               nested: {
-                arrayItem: { value: 'WORLD' },
-                not: 'lost'
+                item: {value: 'WORLD'},
+                not: {value: 'lost'}
               },
-              otherKey: { value: 'Later' }
+              otherKey: {value: 'Later'}
             }, {
               nested: {
-                arrayItem: { value: 'MUNDO' },
-                not: 'lost'
+                item: {value: 'MUNDO'},
+                not: {value: 'lost'}
               },
-              otherKey: { value: 'Hasta luego' }
+              otherKey: {value: 'Hasta luego'}
             }],
-            stays: 'intact'
+            stays: {value: 'intact'}
           }]
         });
+      expect(isFieldValue(state.foo.name)).toBe(true);
+      expect(isFieldValue(state.foo.person.name)).toBe(true);
+      expect(isFieldValue(state.foo.pets[0].name)).toBe(true);
+      expect(isFieldValue(state.foo.pets[1].name)).toBe(true);
+      expect(isFieldValue(state.foo.cats[0])).toBe(true);
+      expect(isFieldValue(state.foo.cats[1])).toBe(true);
+      expect(isFieldValue(state.foo.cats[2])).toBe(true);
+      expect(isFieldValue(state.foo.cats[3])).toBe(true);
+      expect(isFieldValue(state.foo.programming[0].langs[0])).toBe(true);
+      expect(isFieldValue(state.foo.programming[0].langs[1])).toBe(true);
+      expect(isFieldValue(state.foo.programming[0].langs[2])).toBe(true);
+      expect(isFieldValue(state.foo.programming[0].langs[3])).toBe(true);
+      expect(isFieldValue(state.foo.programming[0].langs[4])).toBe(true);
+      expect(isFieldValue(state.foo.programming[1].langs[0])).toBe(true);
+      expect(isFieldValue(state.foo.programming[1].langs[1])).toBe(true);
+      expect(isFieldValue(state.foo.programming[1].langs[2])).toBe(true);
+      expect(isFieldValue(state.foo.programming[1].langs[3])).toBe(true);
+      expect(isFieldValue(state.foo.programming[1].langs[4])).toBe(true);
+      expect(isFieldValue(state.foo.some.numbers[0])).toBe(true);
+      expect(isFieldValue(state.foo.some.numbers[1])).toBe(true);
+      expect(isFieldValue(state.foo.some.numbers[2])).toBe(true);
+      expect(isFieldValue(state.foo.some.numbers[3])).toBe(true);
+      expect(isFieldValue(state.foo.some.numbers[4])).toBe(true);
+      expect(isFieldValue(state.foo.a.very.deep.object.property)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[0].nested.item)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[0].nested.not)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[0].otherKey)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[1].nested.item)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[1].nested.not)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].deeply[1].otherKey)).toBe(true);
+      expect(isFieldValue(state.foo.my[0].stays)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[0].nested.item)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[0].nested.not)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[0].otherKey)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[1].nested.item)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[1].nested.not)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].deeply[1].otherKey)).toBe(true);
+      expect(isFieldValue(state.foo.my[1].stays)).toBe(true);
     });
   });
 });