React: shouldComponentUpdate / render not called after this.setState

[ashtonsix]

This is a bug

Current behavior

Calling this.setState multiple times only re-renders the component once, the value of nextState (inside shouldComponentUpdate which is also called once only) is equal to value passed to setState on the first call.

Expected behavior

shouldComponentUpdate is called each time this.setState is called & the component re-renders if it returns true.

Background

I'm building a library that wraps components & does some state management stuff, the code is similar to react-redux. While working on the context feature I created a BMI calculator to help debug issues. The VDOM for that example looks like this:

Changing the input value should cause the BMICalculatorUsingContext & Input to both re-render, but the Input doesn't. After some investigation I discovered this.setState is called several times inside Mirror(Input), (with the expected values), however shouldComponentUpdate is not called as expected:

To workaround this issue I checked for failures inside the setState callback, & re-called setState if required. Obviously this is not ideal:

this.setState({state, context}, () => {
  if (this.state.state !== state || this.state.context !== context) {
    setTimeout(() => {
      this.setState({state, context})
    })
  }
})

How to replicate

git clone https://github.com/ashtonwar/react-mirror.git
cd react-mirror
git reset --hard b2f4ea539745c770f099666bac8c8557d8f5a601
yarn install
cd examples/BMICalculator
yarn install
yarn start

Go to http://localhost:3000 & change the input value.

While trying to re-create this issue in a simpler example I speculated calling this.setState inside the parent immediately before calling this.setState inside the child caused the problem. Though this idea was incorrect as the child correctly re-rendered & render was called every time this.setState was called. You can see this attempt here.

Using

React 15, Windows 7, Chrome 55

[joni-]

It is stated in the docs that React may batch multiple setState calls into a single update https://facebook.github.io/react/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous.

[ashtonsix]

@joni- Perhaps this explains why shouldComponentUpdate is only called once. But it doesn't explain why nextState.state is equal to the parameters passed to setState for the first time (this.setState({state, context})). The parameters passed reflect the state of an external object & the second screenshot verifies the correct arguments are passed to setState so it seems unlikely the issue arises from my misuse (though this'd be the ideal result ofc).

I would've expected nextState.state to reflect the _last_ call to setState if calls were batched correctly. Perhaps React batched the first four calls correctly (from inside Mirror(Input)) & then the last call was mistakenly dropped / ignored?

[aweary]

@ashtonwar I suspect this is likely because you're indirectly calling setState from shouldComponentUpdate when you dispatch your action in shouldComponentUpdate. Generally you shouldn't be triggering new updates from that lifecycle method, and I think its causing issues.

@gaearon I checked out his demo and it looks like the instance's (Mirror(Input)) pendingStateQueue was not totally flushed. Do you think its because of where setState was triggered? I know we disallow setState calls in other places, should we do the same here?

Also, side note @ashtonwar, your state management system seems to call setState a lot when it doesn't need to. I would try and manage that outside of your HOC somehow so you don't have to rely on shouldComponentUpdate so much.

Lastly, doing a reference equality check between nextState and this.state will always return false; React will create a new object when updating state, so its not a valid way to determine if a component should re-render, if you're re-rendering based on state updates.

[ashtonsix]

@aweary Yes, calling setState inside shouldComponentUpdate seems to be causing the issue, when dispatching the action inside setTimeout (inside shouldComponentUpdate) shouldComponentUpdate is called as expected.

I'm already preventing some calls to setState outside Mirror as you suggested (see here). Getting this right is WIP. But, adding a check to ensure the props actually updated before dispatching UPDATE_PROPS indirectly reduced the number of calls to setState, thanks for bringing my attention to it.

I'm aware nextState & this.state will never be equal after calling this.setState. The component should render every time setState is called (but not necessarily when parent renders or props change).

Before:

shouldComponentUpdate(nextProps, nextState) {
  this.localStore.subscribeParent(nextProps.subscribe)
  this.localStore.dispatch('UPDATE_PROPS', _.omit(nextProps, 'subscribe'))
  return nextState !== this.state
}

After:

shouldComponentUpdate(nextProps, nextState) {
  this.localStore.subscribeParent(nextProps.subscribe)
  if (nextProps !== this.props) {
    setTimeout(() => this.localStore.dispatch('UPDATE_PROPS', _.omit(nextProps, 'subscribe')))
  }
  return nextState !== this.state
}

Repeating steps that demonstrated issue:

[aweary]

@ashtonwar assuming that setState inside shouldComponentUpdate should not be allowed, I don't think there's an issue with React here. As mentioned, setState calls are batched, so you won't get a 1:1 call count for setState and render (and in turn, shouldComponentUpdate).

I'm glad setTimeout resolves this for you, though ultimately I would say you should try and refactor your architecture to avoid triggering updates in shouldComponentUpdate, but that's outside the scope of this issue 😄

We may want to add a note to the documentations about setState in shouldComponentUpdate

[ashtonsix]

@aweary Thanks for the help. I'll leave this issue open (but unsubscribe) so you guys can work out whether you want to disable setState in shouldComponentUpdate, find a way to make it work or update the docs.

[gaearon]

No, setState inside shouldComponentUpdate is not supported.
I guess we could warn for this, but I haven't seen people doing this in practice before.

If we get more reports like this maybe we could do it.

[BruceSuperProgramer]

You didn't see shouldComponentUpdate triggered that is because the screen you had shouldComponentUpdate on it already unmounted. Make sure your app didn't navigate to another screen when you test update life cycles.

[Rotemy]

I'm using this code to delay the render by 250 milliseconds and it is working great.

shouldComponentUpdate(nextProps: Props): boolean { if (nextProps.dataCount < this.props.dataCount) { setTimeout(() => { this.forceUpdate(); }, 250); return false; } return true; }

Is that not supported?