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

Strategy for avoiding cascading renders #125

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

Strategy for avoiding cascading renders #125

ide opened this issue Jun 17, 2015 · 31 comments

Comments

@ide
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@gaearon
Copy link
Contributor

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
Copy link
Contributor

gaearon commented Jun 17, 2015

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

@johanneslumpe
Copy link
Contributor

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

@ide
Copy link
Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link

That's right, it wouldn't.

@gaearon
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

gaearon commented Jun 17, 2015

OK, thanks for explaining!

@ide
Copy link
Author

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
Copy link
Contributor

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
Copy link
Collaborator

acdlite commented Jul 7, 2015

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

@gaearon
Copy link
Contributor

gaearon commented Jul 7, 2015

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

@ide
Copy link
Author

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
Copy link
Contributor

gaearon commented Jul 7, 2015

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

@mindjuice
Copy link
Contributor

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

@acdlite
Copy link
Collaborator

acdlite commented Jul 8, 2015

@gaearon Here ya go! https://github.com/acdlite/redux-batched-updates

@sebmarkbage
Copy link

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
Copy link
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 as completed Jul 8, 2015
@acdlite
Copy link
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
Copy link
Contributor

gaearon commented Jul 30, 2015

Added to docs in 13058f0.

@idibidiart
Copy link

@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
Copy link
Author

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
Copy link

@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.

reduxjs/react-redux#145

https://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?

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

No branches or pull requests

8 participants