Nested fields objects are mutating

[Dr-Nikson]

Hello there! I found some little bug - nested fileds are mutating when you change something:

class MyForm extends PureComponent {
    // ... some stuff
    render() {
        const { fileds: { someField, mainProduct } } = this.props;
        return (
            <div>
                <input {...someField} />
                <MainProductInput fields={mainProduct} />
            </div> 
        );
    }
}

class MainProductInput extends PureComponent {
    // ... some stuff
    render() { 
        // NEVER rerenders because fields objects always the same
        const { fileds: { title, other } } = this.props;
        return (
            <div>
                <input {...title} />
                <input {...other } />
            </div> 
        );
    }
}

reduxForm({
  form: 'mySuperForm',
  fields: [ 'someField', 'mainProduct.title',  'mainProduct.other' ],
})(MyForm)

I mean if you pass down whole nested field object - your children component will never rerenders.
I believe we should map myFields values and create a new objects for all nested fields here ->
https://github.com/erikras/redux-form/blob/master/src/readFields.js#L25

function cloneFields(fields) {
   // mapValues like lodash.mapValues
  return mapValues(fields, field => isNested(field) ? cloneFields(field) : field);
}
const fieldObjects = cloneFields(myFields);

PS guys, I want to tell you really big thanks! You're doing nice job with redux-form)

[Dr-Nikson]

If I'm doing like this:

render() {
        const { fileds: { someField, mainProduct } } = this.props;
        return (
            <div>
                <input {...someField} />
                <MainProductInput fields={ {...mainProduct} } />
            </div> 
        );
    }

Everything works perfect.

[erikras]

Hmm... It is supposed to be creating new instances for any part of the subtree that changes.

Will investigate... 🔍

[aanchalgera]

We are facing the exact issue

[erikras]

Is this still an issue in v4?

[Dr-Nikson]

@erikras I've just checked it - seems issue still here

[luisrudge]

@erikras anything I can do to help you figure this one out?

[kristian-puccio]

Just found this as well

    componentWillReceiveProps(newProps) {
        const isEqual = newProps.fields.customer.email === this.props.fields.customer.email;
        console.log(isEqual, "....isEqual....");
    }

I'll try my hand at a pull-request now and see how I go :)

[erikras]

^^ Look at that test. (it passes) What's wrong about it?

[kristian-puccio]

When I get isEqual = true and I've changed a field that's bad. It means that the field has mutated rather then create a new pointer that I can do a quick reference check against.

One thing I am noticing is that if I change the field so it isn't nested ie just email then I get false "....isEqual....", which is correct.

[erikras]

Okay, but you looked at the test, right? The field that isn't changed is still ===, but the one that does change is !==.

[Dr-Nikson]

@erikras try to add a condition like this:

const address = stub.props.fields.address
// ...
expect(stub.props.fields.address).toNotBe(address)

[erikras]

@iNikNik Very good. That's a bug for sure. Perhaps that's what was originally being referenced. 👍

[luisrudge]

do you think fixing this will help with #529?

[erikras]

@luisrudge Probably not, since this mutating bug is preventing rerendering. Fixing this will result in more rendering.

[luisrudge]

Damn :D

[kristian-puccio]

But it will allow us to wrap components in purerendermixin so only the components that change will be re-rendered.

[kristian-puccio]

Oh and sorry I didn't look at the test. Felt a bit flakey last night so went to bed early. Hopefully tonight I'll get some time.

[erikras]

Fix published as v4.1.15. Please confirm.

[kristian-puccio]

Perfect!

Thanks mate you are awesome

[bbenezech]

@erikras Sorry to bring bad news... :(

  shouldComponentUpdate(nextProps) {
     console.log(this.props.prescription.comment.value === nextProps.prescription.comment.value); // always true
     console.log(this.props.prescription === nextProps.prescription); // always true
  }

When I update comment, both this.props.prescription.comment.value and nextProps.prescription.comment.value are equal in value and object (value is correct), which would mean that this.props.prescription has been mutated into nextProps.prescription.

I use this.props.fields.prescriptions.addField(prescription);, prescriptions is an array at the root of my reduxForm.

          {this.props.fields.prescriptions.map((prescription, index) =>
            <Prescription
              key={index}
              prescription={prescription}
              index={index}
            />
          )}

I use the latest release.

And when I check values, this.props.values.prescriptions[0] === nextProps.values.prescriptions[0] it is false when I modify other elements in the array. (opposite problem).

[bbenezech]

Ok, I wrote a spec for both issues. @erikras Can you have a look?

  it('should not mutate tree or change untouched nested values in arrays', () => {
    const store = makeStore();
    const form = 'testForm';
    const Decorated = reduxForm({
      form,
      fields: ['phones[].prefix', 'phones[].number']
    })(Form);
    const dom = TestUtils.renderIntoDocument(
      <Provider store={store}>
        <Decorated/>
      </Provider>
    );
    const stub = TestUtils.findRenderedComponentWithType(dom, Form);

    stub.props.fields.phones.addField();
    stub.props.fields.phones.addField();

    const phones = stub.props.fields.phones;
    const values = stub.props.values;
    const phone0 = phones[0];
    const phone1 = phones[1];

    phone0.number.onChange('555-1234');

    expect(stub.props.values).toNotBe(values); // works
    expect(stub.props.values.phones).toNotBe(values.phones); // works
    expect(stub.props.values.phones[0]).toNotBe(values.phones[0]); // works
    expect(stub.props.values.phones[1]).toBe(values.phones[1]); // fails. useless update?

    expect(stub.props.fields.phones).toNotBe(phones); // fails. tree mutation?
    expect(stub.props.fields.phones[1]).toBe(phone1); // good, but...
    expect(stub.props.fields.phones[0]).toNotBe(phone0); // fails. tree mutation?
  });

I guess if you can make that test pass, you would be some kind of Pure-Render superhero.

[Coobaha]

@erikras Hi!

Right now you mutate object primitives, for phones[].number and phones[0].number.onChange().
Phones array and all its members have same references (same objects),and only field primitive props are mutated.

So this evaluates to true: nextProps.fields === this.props.fields && nextProps.fields.phones === this.props.fields.phones && nextProps.fields.phones[0] === this.props.fields.phones[0].

Should fields and values be new objects/arrays on their part change, like you will do in the reducer on state change?

fields: {
...fields,
phones: [
    {
    ...phones[0],
    value: newValue,
    touched: true,
    (etc...)
    },
    ...without(phones, phone[0])
]},

This will allow pure rendering, and also passing whole deep form slices to components. Right now I am supposed to render whole fields in wrapped component.

Btw, props.values are changed on focus/blur events, should they? Imo they should be new object for corresponding path only if value in this path changed, like fields above.

P.S. Sorry if this is already some where in issues, I did not found them.

[villanuevawill]

@erikras You're the man, and thanks for this library! Would you mind addressing the two questions above please? Was this fixed in a later release? I'm still on version 4 and getting ready to update, but want to make sure if the SPEC @bbenezech would pass or @Coobaha 's question. My app slows down because everything must re-render since the fields array shows it remains equal.

[erikras]

Published fix in v5.3.4.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.