Strategy for avoiding cascading renders #125

Closed
ide opened this Issue Jun 17, 2015 · 31 comments

Comments

8 participants
@ide

ide commented Jun 17, 2015

As I understand it, when you update a Store it broadcasts an update to all Connectors. Each Connector selects the slice of State that its interested in and re-renders itself by calling this.setState. React then recursively renders the component hierarchy until it hits a nested Connector, whose componentShouldUpdate method compares the old and new props (it also compares the Connector's State, which is guaranteed not to have changed because (a) it's a slice of a Store's immutable State and (b) this nested Connector hasn't called this.setState). If the props have changed, React continues rendering the component hierarchy. Once React is done, the second, nested Connector receives an update from the Store, selects its slice of State, and re-renders itself. In the worst-case scenario, a linear chain of n Connectors could result in O(n^2) render calls.

This is a small example that demonstrates this cascading render effect. Both Connectors are subscribed to the same slice of State, and the parent passes down a prop to the child.

type StoreState = {
  user: {
    name: string;
    age: number;
  };
};

@connect(state => ({ user: state.user }))
class Parent extends Component {
  render() {
    let { user } = this.state;
    let displayName = (user.age < 18) ? getFirstName(user.name) : user.name;
    return <Child displayName={displayName} />;
  }
}

@connect(state => ({ user: state.user }))
class Child extends Component {
  render() {
    let { user } = this.state;
    return (
      <ul>
        <li>{this.props.displayName}</li>
        <li>{this.props.age}</li>
        {__DEV__ && <li>this.state.name</li>}
      </ul>
    );
  }
}

This is what I'm currently thinking:

  1. The Connectors' subscriptions to the Redux instance need to be ordered such that a parent's subscription is before its children's. I believe this is already the case so we're good here.
  2. New: when a Connector is re-rendered, it selects its slice of State from the Stores and assigns it to this._state. This means a Connector always renders with the most up-to-date State even if it hasn't been notified of a change yet. It also means that Store updates are atomic from the perspective of the subscribing views... currently a Connector's props may be derived from the new State while the Connector's state is a slice of the old State.
  3. New: when the Connector is notified of a change, it selects its slice of State from the Store and compares it against this._state. If there's no change (e.g. because we already got an up-to-date slice when the parent re-rendered this Connector) then do nothing. If there is a change then we update this._state and call this.forceUpdate.

The main thing I'm going for is the atomic Store update from the perspective of the React components. From Redux's perspective, the updates are atomic in the sense that it doesn't notify subscribers till the Stores are done transforming the State. But from the perspective of a connected component it's receiving a mix of old and new State as props, while in traditional Flux the components always read a consistent view of the Stores.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

Thanks for posting this. Keeping this open for now. I'll revisit after ReactEurope.
If somebody wants to help tackle this please write here :-)

Collaborator

gaearon commented Jun 17, 2015

Thanks for posting this. Keeping this open for now. I'll revisit after ReactEurope.
If somebody wants to help tackle this please write here :-)

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 17, 2015

If you run into someone from Relay it could be good to get their input since this is the kind of performance problem faced by a big website with many components reading from the same stores.

ide commented Jun 17, 2015

If you run into someone from Relay it could be good to get their input since this is the kind of performance problem faced by a big website with many components reading from the same stores.

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe Jun 17, 2015

Collaborator

@ide some good thoughts there, it would definitely be a welcome improvement, as performance increases are always a good thing :)

Collaborator

johanneslumpe commented Jun 17, 2015

@ide some good thoughts there, it would definitely be a welcome improvement, as performance increases are always a good thing :)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

If you run into someone from Relay it could be good to get their input since this is the kind of performance problem faced by a big website with many components reading from the same stores.

Related: facebook/react#3920 (comment)

Collaborator

gaearon commented Jun 17, 2015

If you run into someone from Relay it could be good to get their input since this is the kind of performance problem faced by a big website with many components reading from the same stores.

Related: facebook/react#3920 (comment)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

Can't we just wrap the subscription notification in React.addons.batchedUpdates by the way?

Collaborator

gaearon commented Jun 17, 2015

Can't we just wrap the subscription notification in React.addons.batchedUpdates by the way?

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe Jun 17, 2015

Collaborator

@ide that's what you suggested to me the other day in regard to flummox, right?

Collaborator

johanneslumpe commented Jun 17, 2015

@ide that's what you suggested to me the other day in regard to flummox, right?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 17, 2015

I don't know the behavior of batchedUpdates across components though it may solve this problem. Will have to look at its source.

In flummox's case one component might subscribe to multiple stores that each emit a change in response to the same action, so batching that component's setState calls is useful. With redux components have just one subscription to the redux instance and all the stores update before the component is notified once per action.

ide commented Jun 17, 2015

I don't know the behavior of batchedUpdates across components though it may solve this problem. Will have to look at its source.

In flummox's case one component might subscribe to multiple stores that each emit a change in response to the same action, so batching that component's setState calls is useful. With redux components have just one subscription to the redux instance and all the stores update before the component is notified once per action.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

I don't know the behavior of batchedUpdates across components though it may solve this problem. Will have to look at its source.

It reaches into React's internals and tells it that setState shouldn't immediately update but instead should wait until the callback is over. This is exactly what React already does when dispatching DOM events.

Isn't this the kind of thing you were talking about? Both parent and child are invalidated, but the DOM changes are not cascading and instead applied in one pass.

Perhaps @spicyj can confirm if that's indeed how batchedUpdates works.

Collaborator

gaearon commented Jun 17, 2015

I don't know the behavior of batchedUpdates across components though it may solve this problem. Will have to look at its source.

It reaches into React's internals and tells it that setState shouldn't immediately update but instead should wait until the callback is over. This is exactly what React already does when dispatching DOM events.

Isn't this the kind of thing you were talking about? Both parent and child are invalidated, but the DOM changes are not cascading and instead applied in one pass.

Perhaps @spicyj can confirm if that's indeed how batchedUpdates works.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 17, 2015

Yes, that's what batchedUpdates does. Parents will reconcile first so that the children render only once (with the newest props from their parent and their newest state).

Yes, that's what batchedUpdates does. Parents will reconcile first so that the children render only once (with the newest props from their parent and their newest state).

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

Thanks!

The un-nice thing about it is that it's an addon, which means we can't get it from React. We can't get the internal ReactUpdates from React either. Ugh..

I wonder if 0.14 solves that somehow.

Collaborator

gaearon commented Jun 17, 2015

Thanks!

The un-nice thing about it is that it's an addon, which means we can't get it from React. We can't get the internal ReactUpdates from React either. Ugh..

I wonder if 0.14 solves that somehow.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 17, 2015

You'll be able to require react/addons/batchedUpdates (and avoid packaging the other addons) but there's no way to get it from the React object, sorry. I think @sebmarkbage may have wanted to move it to React sometime though?

You'll be able to require react/addons/batchedUpdates (and avoid packaging the other addons) but there's no way to get it from the React object, sorry. I think @sebmarkbage may have wanted to move it to React sometime though?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

Can a library depend on anything from react/addons/*? I think that wouldn't work if the user aliases react to a precompiled file, would it?

Collaborator

gaearon commented Jun 17, 2015

Can a library depend on anything from react/addons/*? I think that wouldn't work if the user aliases react to a precompiled file, would it?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 17, 2015

That's right, it wouldn't.

That's right, it wouldn't.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

It's unfortunate batchedUpdates is not on React itself then. An addon by definition, in my opinion, should be implementable outside the library, but batchedUpdates is privileged to expose an internal.

Collaborator

gaearon commented Jun 17, 2015

It's unfortunate batchedUpdates is not on React itself then. An addon by definition, in my opinion, should be implementable outside the library, but batchedUpdates is privileged to expose an internal.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 17, 2015

We think of it slightly differently – addons use React internals (which is why we package them at all instead of letting someone else write it), but their API is liable to change more frequently.

We think of it slightly differently – addons use React internals (which is why we package them at all instead of letting someone else write it), but their API is liable to change more frequently.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 17, 2015

Collaborator

OK, thanks for explaining!

Collaborator

gaearon commented Jun 17, 2015

OK, thanks for explaining!

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 17, 2015

Parents will reconcile first so that the children render only once (with the newest props from their parent and their newest state).

Cool - sounds like batchedUpdates addresses this then.

I made a little demo with two nested components that read the same store data and to my surprise the updates already appear batched (the child gets its new props and new state together). https://gist.github.com/ide/1da8a70a14535835da30

ide commented Jun 17, 2015

Parents will reconcile first so that the children render only once (with the newest props from their parent and their newest state).

Cool - sounds like batchedUpdates addresses this then.

I made a little demo with two nested components that read the same store data and to my surprise the updates already appear batched (the child gets its new props and new state together). https://gist.github.com/ide/1da8a70a14535835da30

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 7, 2015

Collaborator

@ide I think this is because React batches updates coming from events by default.
@acdlite Sounds like batchedUpdates is a great fit for a middleware that wraps inner dispatch calls in it.

Collaborator

gaearon commented Jul 7, 2015

@ide I think this is because React batches updates coming from events by default.
@acdlite Sounds like batchedUpdates is a great fit for a middleware that wraps inner dispatch calls in it.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 7, 2015

Collaborator

@gaearon Would there be any downside in a middleware that wraps all dispatches in batchedUpdates?

Collaborator

acdlite commented Jul 7, 2015

@gaearon Would there be any downside in a middleware that wraps all dispatches in batchedUpdates?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 7, 2015

Collaborator

@acdlite I don't think so, that's what I'd do.

Collaborator

gaearon commented Jul 7, 2015

@acdlite I don't think so, that's what I'd do.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jul 7, 2015

@gaearon You're right -- React Native wasn't the best environment to test in because it batches messages from the bridge :P

ide commented Jul 7, 2015

@gaearon You're right -- React Native wasn't the best environment to test in because it batches messages from the bridge :P

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 7, 2015

Collaborator

@ide React will also batch for DOM events. But it won't for AJAX callbacks etc.

Collaborator

gaearon commented Jul 7, 2015

@ide React will also batch for DOM events. But it won't for AJAX callbacks etc.

@mindjuice

This comment has been minimized.

Show comment
Hide comment
@mindjuice

mindjuice Jul 7, 2015

Contributor

NuclearJS just added a batch() function, since React was previously notified after EVERY action: optimizely/nuclear-js@de35e19

Contributor

mindjuice commented Jul 7, 2015

NuclearJS just added a batch() function, since React was previously notified after EVERY action: optimizely/nuclear-js@de35e19

@acdlite

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 8, 2015

There are some unsolved issues with batching such as with controlled components. The plan is likely to move to batching by default with an opt-out for difficult cases.

There are some unsolved issues with batching such as with controlled components. The plan is likely to move to batching by default with an opt-out for difficult cases.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 8, 2015

Collaborator

@sebmarkbage Great to hear, that sounds sensible.

Closing this since the extension I posted above addresses the issue.

Collaborator

acdlite commented Jul 8, 2015

@sebmarkbage Great to hear, that sounds sensible.

Closing this since the extension I posted above addresses the issue.

@acdlite acdlite closed this Jul 8, 2015

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 8, 2015

Collaborator

On second thought let's keep this open until we have docs surrounding it.

Collaborator

acdlite commented Jul 8, 2015

On second thought let's keep this open until we have docs surrounding it.

@acdlite acdlite reopened this Jul 8, 2015

@acdlite acdlite added the docs label Jul 8, 2015

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 30, 2015

Collaborator

Added to docs in 13058f0.

Collaborator

gaearon commented Jul 30, 2015

Added to docs in 13058f0.

@gaearon gaearon closed this Jul 30, 2015

@epeli epeli referenced this issue in reduxjs/react-redux Sep 9, 2015

Merged

Fix issues with stale props #99

@idibidiart

This comment has been minimized.

Show comment
Hide comment
@idibidiart

idibidiart Jan 12, 2016

@ide @gaearon i know this is closed but for educational purposes what are the reasons someone would want to connect the child components rather than push the root parent state as props down the component tree? This way only the root parent is connected. This is heirarchical composition, so descendants should all be stateless components, no? For example, a calendar date selection component would have one stateful parent and a bunch of stateless components as descendants. Connected state is pushed down via read only props from calendar elemt root component to the statelsss descendants. What am I missing?

@ide @gaearon i know this is closed but for educational purposes what are the reasons someone would want to connect the child components rather than push the root parent state as props down the component tree? This way only the root parent is connected. This is heirarchical composition, so descendants should all be stateless components, no? For example, a calendar date selection component would have one stateful parent and a bunch of stateless components as descendants. Connected state is pushed down via read only props from calendar elemt root component to the statelsss descendants. What am I missing?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jan 12, 2016

I think of it as a matter of grouping related code or concerns together. In contrast, consider an approach like Relay's where many components are connected and the components and their data requirements (ex: a Redux selector or Relay query) exhibit high cohesion.

Imagine if a site like GitHub were implemented with Redux and React. The component hierarchy would likely be many levels deep. For example, the IssueCommentHeader component would be quite far away from the root component. If only the root component were connected to the store, the root would need to know that the IssueCommentHeader—very far away—needs the user's username and profile picture.

What if you change IssueCommentHeader so that it needs the user's online/offline presence, like a chat app? Now you need to modify IssueCommentHeader's owner, and its owner, and so on until you've modified every component up to the root so that it threads the user's presence data down this deep component hierarchy.

In summary it's not scalable for your productivity, let alone a team's productivity, to put all of the data-retrieving functionality in the root component. This was a real problem when building the liking & commenting UI on Facebook—it looks like a small component but there's a lot of complexity hiding inside it and for its root component to need to know all the data wanted by its leaves deep down was hard to write code for and maintain. What we really wanted is for components at many levels of the hierarchy to specify the data they needed so that related code was grouped together.

ide commented Jan 12, 2016

I think of it as a matter of grouping related code or concerns together. In contrast, consider an approach like Relay's where many components are connected and the components and their data requirements (ex: a Redux selector or Relay query) exhibit high cohesion.

Imagine if a site like GitHub were implemented with Redux and React. The component hierarchy would likely be many levels deep. For example, the IssueCommentHeader component would be quite far away from the root component. If only the root component were connected to the store, the root would need to know that the IssueCommentHeader—very far away—needs the user's username and profile picture.

What if you change IssueCommentHeader so that it needs the user's online/offline presence, like a chat app? Now you need to modify IssueCommentHeader's owner, and its owner, and so on until you've modified every component up to the root so that it threads the user's presence data down this deep component hierarchy.

In summary it's not scalable for your productivity, let alone a team's productivity, to put all of the data-retrieving functionality in the root component. This was a real problem when building the liking & commenting UI on Facebook—it looks like a small component but there's a lot of complexity hiding inside it and for its root component to need to know all the data wanted by its leaves deep down was hard to write code for and maintain. What we really wanted is for components at many levels of the hierarchy to specify the data they needed so that related code was grouped together.

@idibidiart

This comment has been minimized.

Show comment
Hide comment
@idibidiart

idibidiart Jan 13, 2016

@ide

I researched these questions by Dan himself (on S.O.) and users of Redux grappling with the same exact issue, so I am glad I'm not the first to ask.

rackt/react-redux#145

http://stackoverflow.com/questions/25701168/at-what-nesting-level-should-components-read-entities-from-stores-in-flux

I think the answer on S.O. is not ideal... leaving it to the programmer is not a cozy solution

@gaearon have you any updated insight?

@ide

I researched these questions by Dan himself (on S.O.) and users of Redux grappling with the same exact issue, so I am glad I'm not the first to ask.

rackt/react-redux#145

http://stackoverflow.com/questions/25701168/at-what-nesting-level-should-components-read-entities-from-stores-in-flux

I think the answer on S.O. is not ideal... leaving it to the programmer is not a cozy solution

@gaearon have you any updated insight?

@peteruithoven peteruithoven referenced this issue in tappleby/redux-batched-subscribe Jan 15, 2016

Closed

Combine batchedUpdates & requestAnimationFrame? #5

@peteruithoven peteruithoven referenced this issue in tappleby/redux-batched-subscribe Nov 25, 2016

Open

Adding exceptions #16

@jebeck jebeck referenced this issue in tidepool-org/viz Dec 5, 2016

Merged

CGM Trends Range Selections #36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment