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

sCU doesn't prevent child rerender when legacy context and batched setStates are involved #13920

Closed
errendir opened this issue Oct 23, 2018 · 2 comments

Comments

@errendir
Copy link

Do you want to request a feature or report a bug?
This is a bug report. I think this bug and one already reported (#13856) have the same root cause.

What is the current behavior?
A component instance preventing its own rerender, by returning false from shouldComponentUpdate may under certain condition still get its children rerendered, even if no direct update to said children happened.

This behaviour is triggered by a very specific stack of components. The simplest example I managed to create (https://codesandbox.io/s/9zymvq479r) involves five components, each rendering the next one:

  • UpdatingRoot responsible for triggering the top-level update, by calling setState in the componentDidMount lifecycle method.
  • LegacyContextInjector responsible for creating the child context object according to the legacy context API.
  • SCUBarrier with shouldComponentUpdate function always returning false. Any children of this component instance should not be rerendered as long as they themselves are not sheduled for an update. This is the behaviour I expect. This bug prevents it from happening.
  • IntermediateComponent responsible for rendering UpdatingChild few levels down in the component instance tree.
  • UpdatingChild responsible for calling its own setState in the componentDidMount lifecycle method.

Each of the created component instances will be rendered only one or two times. Both UpdatingRoot and UpdatingChild are expected to be rendered twice, and this does happen. Each of the IntermediateComponent instances is expected to render only once. This expectation is based on the fact that the SCUBarrier#shouldComponentUpdate always returns false, and no child IntermediateComponent instance updates itself. The legacy context API should not pierce through shouldComponentUpdate returning false (https://reactjs.org/docs/legacy-context.html#updating-context).

Despite those expectation, all IntermediateComponent instances are rendered twice.

It's important that the UpdatingRoot#setState and UpdatingChild#setState function are batched together. You can try "unbatching" them, by swapping:

this.setState({ weAreHome: "my friend" });

for

setTimeout(() => this.setState({ weAreHome: "my friend" }),0)

When this is done the IntermediateComponent instances are correctly rendered only once.

My example code does the batching by calling UpdatingRoot#setState and UpdatingChild#setState functions directly in the componentDidMount lifecycles of their respective components. I believe this generally is an anti-pattern, but this approach was chosen for simplicity. The same issue can be replicated by calling both setState in ReactDOM.unstable_batchedUpdates(() => { ... })

For a more practical example of a situation in which two ancestor-descendant components may naturally schedule updates in a batch, take a look at https://github.com/tappleby/redux-batched-subscribe.

Legacy context API is used by popular components like Transition from https://github.com/reactjs/react-transition-group or by commonly used react-redux.

A real-life example of this bug could be:
Two Redux-connected component A and C are scheduled for update in the same batch. A Transition (from react-transition-group) injecting legacy context is rendered between them. Some component B in between A and C (descendent of A, ancestor of C) implements shouldComponentUpdate to optimize the rendering of some unchanging subtree (which eventually, down the line, renders B). Both A and C component instances rerender correctly (as expected, since react-redux uses legacy context API correctly as described in https://medium.com/@mweststrate/how-to-safely-use-react-context-b7e343eff076). However the instances corresponding to the unchanging element tree returned by B will also be unexpectedly rerendered.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
https://codesandbox.io/s/9zymvq479r

What is the expected behavior?
All IntermediateComponent instances should be rendered only once.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
react "16.6.0-alpha.f47a958"
react-dom "16.6.0-alpha.8af6728 - canary"

I originally discovered this issue in the React/ReactDOM version 16.5.2

Please let me know if there is something unclear about this bug. It's definitely not an easy thing to reproduce, since so many different components need to be involved!

@gaearon
Copy link
Collaborator

gaearon commented Oct 23, 2018

Like #13856, we can take a fix if somebody sends one but it's unlikely we'll be working on this since the legacy context is being phased out.

@aweary
Copy link
Contributor

aweary commented Oct 23, 2018

Closing as a duplicate of #13856, since they're probably symptoms of the same root issue.

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

3 participants