[5.1.0-test.1] Connected components don't re render after state update

[ematipico]

I gave a first try to the new beta and I can see some problems (unless I should make some changes).

The components that are connected to the store via connect don't get updated (they don't re render). While debugging I can see that the function mapStateToProps gets called correctly (and I can see the changes) after the action but the component doesn't re-render hence it doesn't show the new values.

I hope it could help

[markerikson]

Thanks. Can you put together an example reproducing the issue?

[ematipico]

Closing, as the problem was probably due to an internal change of mine. Will re-open if I can understand the issue.

EDIT: Actually it's not, previous version works and new version doesn't work. Will try to figure out what's going on and provide some example if I can

[gaearon]

My guess is that getDerivedStateFromProps usage pattern is incorrect. Lack of comparisons in it is concerning. The tests probably don’t catch it because they’re not using 16.4 yet.

It’s worth reading https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html and applying that information to latest changes.

[gaearon]

(I’m referring to React Redux implementation, not your application code)

[cellog]

@gaearon is correct. The current implementation calls gDSFP when props change, and then the updater changes the state, which causes gDSFP to re-run, which finds that the state is the same, which sets shouldComponentUpdate to false, and nothing re-renders.

so we have:

props change -> getDerivedStateFromProps -> runUpdater -> setState(props, shouldComponentUpdate: true) -> getDerivedStateFromProps -> runUpdater -> setState(shouldComponentUpdate: false) -> shouldComponentUpdate -> returns false

because of this, react-redux can't store the derived props in state and shouldn't be using gDSFP at all. Instead, I think that Connect should be managing the 2 possible mutations independently. The store subscription can simply update the state to the new store state to force a re-render check, and we run updater in shouldComponentUpdate to retrieve the derived props from the new props and the store state. Once the derived props have been generated, we can store them to use in render().

As I'm relatively unfamiliar with the internals of react-redux, I have a fix in a fork, but I'm working on making existing tests pass before making a pull request.

I have one question regarding one of the tests. in connect.spec, in test should pass state consistently to mapState I see that we have a case where batched updates don't actually trigger a store change before subscriptions are updated. Specifically:

      // The store state stays consistent when setState calls are batched
      ReactDOM.unstable_batchedUpdates(() => {
        store.dispatch({ type: 'APPEND', body: 'c' })
      })
      expect(childMapStateInvokes).toBe(2)

Why is it expected that child mapStateToProps would be called? In this case, the props and the state remain unchanged. It seems to me that this test is wrong. Before I change this, I wonder if the experts could weigh in? Shouldn't we only run mapStateToProps to generate derived props when either (1) the connected component's props change or (2) the store's state is changed? Not on (3) when any action is dispatched? My assumption here is that mapStateToProps is always a pure function, but maybe I'm missing something?

Thanks in advance

[markerikson]

@cellog : thanks for the research and the PR! I'm off on a vacation atm, but I've been meaning to take a day to try pushing some of the React-Redux work forward. No guarantees on a timeline, but this is definitely now on my list to look at.

It looks like that test was added to fix #86 . At that point, React-Redux still had problems with connected children. The classic example is a connected list with connected list items, and if you deleted the data for a list item, the list item's mapState would run before the parent re-rendered and potentially explode because the data it needed was no longer in the store. This was resolved in v5 by enforcing top-down subscription behavior. It does look like the test has been tweaked some over time as a couple other assumptions have changed. To be honest, though, I'm not entirely sure what the test is actually proving in relation to batching, especially since it's only dispatching once inside the unstable_batchedUpdates() call. @gaearon , @epeli , can either of you comment on that?

Per your questions at the end, here's the expected sequence when an action is dispatched as of v5:

• Root reducer runs
• Subscriber functions all run, which includes a separate subscriber callback for each connected component instance.
• Each connect callback checks to see if prevStoreState === currentStoreState. If they're the same, it assumes that the action did not result in store state updates, and bails out.
• Assuming the root state has changed, connect then calls your mapState function, and does a shallow equality check on the returned object to determine if a re-render is needed.

Sequence in this test:

• Action is dispatched, state is updated
• Parent mapState runs, parent re-renders
• Child still exists, child mapState runs, return value is an empty object, no need to re-render

So sure, I'd expect the child's mapState to have executed once for that one action, but I'm not sure what that proves.

[esamattis]

I'm travelling at the moment and unable checkit for a week or so as don't remember it on top of my head. I'll check on this when I'm home again.

[cellog]

FYI, I don't think instance properties are async-unsafe. In the memoization example:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

class Example extends Component {
  // State only needs to hold the current filter text value:
  state = { filterText: "" };

  // Re-run the filter whenever the list array or filter text changes:
  filter = memoize(
    (list, filterText) => list.filter(item => item.text.includes(filterText))
  );

  handleChange = event => {
    this.setState({ filterText: event.target.value });
  };

  render() {
    // Calculate the latest filtered list. If these arguments haven't changed
    // since the last render, `memoize-one` will reuse the last return value.
    const filteredList = this.filter(this.props.list, this.state.filterText);

    return (
      <Fragment>
        <input onChange={this.handleChange} value={this.state.filterText} />
        <ul>{filteredList.map(item => <li key={item.id}>{item.text}</li>)}</ul>
      </Fragment>
    );
  }
}

The recommended approach uses an instance variable filter

So that is not an issue.

[markerikson]

Hmm. Interesting.

@gaearon , @bvaughn , @acdlite : can any of you provide some clarification around use of component instance variables and async safety?

[bvaughn]

FYI, I don't think instance properties are async-unsafe.

Instance properties can be tricky to get right.

Function bindings are okay. (In fact they're often necessary!) But be wary of this.props or this.state references inside of your bound functions, because they might hold stale values (if the component has been rendered but not yet committed, or if an error occurred while rendering). If you need to access something in e.g. component state, use a setState updater function and read from the parameter passed to it as mentioned in the docs.

Other instance properties can be okay as well, although I would suggest avoiding them when possible. If you look at our create-subscription package, for example, you'll see that we use a couple of instance variables in it. The key here is to only update these values from a commit phase lifecycle (componentDidMount or componentDidUpdate) to ensure they are correct.

Does this clarify? I can elaborate if not.