Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For certain child components, triggering an onChange and updating redux and/or state does not re-render #206

Closed
ssmith353 opened this issue Feb 6, 2017 · 7 comments

Comments

@ssmith353
Copy link

Hi MetalJS Team,
We're having an issue where even if we set the state or update Redux with an onChange, the component does not re-render. Showing stale data.

Only if we click in and out of another component does the original component re-render, and show the correct new value.

Notice in the attached video that when a new item is selected, the "selected" value does not change, though redux does update. But when we click in - and - out of the input box below, the input above updates. Note: This component is wrapped with the metal-react bridge.

goo.gl/nTtZM1

@ssmith353
Copy link
Author

@ssmith353
Copy link
Author

@mthadley
Copy link

mthadley commented Feb 6, 2017

If possible, it would be awesome if you could provide some kind of minimal example that reproduces the bug (outside of your application). It's also a good thing to do since in the process you might find the core problem (whether or not the bug is in Metal or your own application).

@ssmith353
Copy link
Author

ssmith353 commented Mar 14, 2017

@mthadley, Attached is a simple Redux app (that mirrors our project structure) and demo's our issue. Noticing that if you have the redux console open, the changes are propagated, but does not trigger a re-render of the form, and does not pass a new value to the child components.

Note: This same issue only occurs for ONE (instance) of a specific component, but works for every other instance of even the same component (in our project).

Redux Demo App

@mthadley
Copy link

@ssmith353 I'll try and take a look when I have some time! 👍

@mthadley
Copy link

mthadley commented Mar 14, 2017

@ssmith353

So I think I found the issue. First, try changing your connect call to this at the bottom FormContainer.js:

export default connect(
  mapStateToProps,
  mapDispatchToProps,
  null,
  {
    pure: false
  }
)(FormContainer);

You should see it working now. So I think what was happening is that you were mutating and calling setIn with the same formData object. connect does some performance optimizations like checking if the store actually changed, or if your previous store props equal the next store props. Because you are passing around the same formData object, these checks cause the component to skip re-rendering. By adding {pure: false}, you are telling metal-redux to skip these checks.

This can also be verified by removing the previous changes, and then changing the call in handleUpdateData to this:

/* calling with a *new* Object each time */
updateFormData(Object.assign({}, formData), 12345); 

So the solution is not to add the pure flag, but to instead make sure that all of the data in your store is actually immutable. Inside of your reducer you should instead call setIn with fromJS:

import {fromJS} from 'immutable';

/* ... */

return state.setIn(['formData', id], fromJS(data));

and then make sure to call toJS inside of your FormContainer component.

render() {
  const {formData, updateFormData} = this.props;

  return (
    <div class="form-container">
      <h1> Form Demo </h1>

      <Form
        formData={formData.toJS()}
        updateFormData={updateFormData}
      />
    </div>
  );
}

Let me know if you are still having issues, and we can maybe talk over slack about it.

P.S. Also, as a side note, you can cut down on some of the boilerplate in your connect by doing something like this:

export default connect(
  /* The second argument for most Immutable.js methods is a default value */
  state => (
    {
      formData: state.getIn(['form', 'formData', 12345], Map());
    }
  ),
  /* The second argument to connect can also be an object,
  *  it will auto-wrap your actions with dispatch.
  */
  {
    updateFormData
  }
)(FormContainer);

@ssmith353
Copy link
Author

That worked! Thanks @mthadley

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants