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

unimplement shouldComponentUpdate? #507

Closed
jimbolla opened this issue Sep 28, 2016 · 23 comments

Comments

8 participants
@jimbolla
Copy link
Contributor

commented Sep 28, 2016

There was some good discussion on Twitter (between @gaearon, @markerikson, @timdorr, @mjackson, @ryanflorence, et al.) about connect's component implementing shouldComponentUpdate.

If I understand correctly, the main issue being that any component wrapped in connect then blocks updates related to context changes, and this has an adverse impact on libraries that pass data via context that need to trigger rerenders (such as React Router's Link component.)

The changes in the next branch of React Redux make its implementation of shouldComponentUpdate much less necessary, mainly because now setState is no longer called as a response to every store state change, but only if the final merged props have changed. So now when shouldComponentUpdate is called as a result of calling setState, it's always going to return true anyways. (The call to setState could probably be replaced with forceUpdate and would work exactly the same.)

In the case of shouldComponentUpdate being called after receiving new props from parent, it's effectively just acting like PureComponent. That responsibility can be given to the components being wrapped, which would have better knowledge about if/how they should implement shouldComponentUpdate. I personally would use recompose and do something like:

export default compose(
   pure, // <-- from recompose
   connect(...),
)(MyComponent);

An alternative to removing connect's shouldComponentUpdate completely would be to make it an another option argument, and decide whether it should be opt-in or opt-out.

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2016

Oh boy, this oughta be good... :)

@timdorr

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

Just to be clear and make sure everyone understands what's going on, here is the situation with something like react-redux and React Router:

<Router>
  <connect(App)>
    <Link to="/foo" activeClassName="active">

In this case, <Router> is providing a context that <Link> is consuming something like this:

let className
if (this.context.location == this.props.to) 
  className = this.props.activeClassName

(That's a crazy over-simplification, but it gets the point across)

If the URL/location on the page changes, then that information should propagate from the <Router> to the <Link> via this.context, causing it to potentially re-render. Context is a way to cross the boundaries of direct Component-to-Component communication via props.

The problem lies with shouldComponentUpdate not being able to take into account changes on this.context. The connect()ed component thinks nothing has changed with its props during this time (which is true!), so it tells React that it shouldn't re-render this component (or anything underneath it, including the <Link>!).

Hopefully that explains the situation well enough. How to fix it is another matter :)

@mjackson

This comment has been minimized.

Copy link

commented Sep 28, 2016

I think react-redux's use of sCU is fine. We've already got an issue open on the router (v4 branch) to stop using context to communicate state changes to <Link>s. This should make it much easier to use the router in a redux app.

@timdorr

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

So, I'm wondering if you could store the props for the WrappedComponent in state (thereby creating the comically confusing this.state.props) and then rather than on this.selector outside of React's knowledge.

Basically, you would change this line to store this.selector.props in state. You still get the appropriate update behavior, but you don't block things underneath.

BTW, there's already a potential opt-in option: pure. We can use that to determine if we should or should not turn on sCU usage.

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

@timdorr I don't think changing where the computed props are stored would unblock things. The issue is that the implementation of shouldComponentUpdate can't do "the computed props have changed or something in context has changed". I'm pretty sure there are some open issues in React about adding new methods for components to communicate context changes, but that doesn't help us today.

@timdorr

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

Actually, that is correct, though it would be a bit more idiomatic React to store something in setState, rather than use it as a workaround to forceUpdate.

But sCU needs to be removed for it to stop interrupting context propagation. I don't think there would be any problem with doing that right now since the props are stored and precomputed by the selector.

@jimbolla jimbolla added the enhancement label Oct 4, 2016

@timdorr timdorr added this to the 5.0 milestone Oct 5, 2016

@levity

This comment has been minimized.

Copy link

commented Oct 19, 2016

Does adding this to the 5.0 milestone mean that it will be done in 5.0? I'd like to see it for reasons unrelated to React Router: I keep the current user in context in my app and make changes to it (e.g. changing the profile picture).

@timdorr

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

@jimbolla How about we do this as a test release (5.0.0-test.1) and see what the effects the community can find? I'd also love to have a benchmark suite available so we can have an objective analysis on our own, but that could work in a pinch until we get something built out.

@jimbolla jimbolla referenced this issue Oct 24, 2016

Closed

v5 release chores #473

7 of 7 tasks complete
@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

I'm not opposed to that. But do we want to remove sCU completely or make it opt-in or opt-out? The code changes are easy either way, but tests will need tweaked and docs updated.

@timdorr

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

I'd go with opt-in. Most users don't need it and it's an optimization that comes with side effects, so turning it on should be a conscious decision by the user. Plus, we can document those side effects to warn people about it ahead of time.

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

Ok. That should be easy enough to implement. What should the option be called?

@timdorr

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Can it be combined/tacked-on to the pure option perhaps?

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

Probably not. You usually want pure on and this new option off. Setting pure to false means every store state change triggers a rerender for every connected component.

@primozs

This comment has been minimized.

Copy link

commented Nov 3, 2016

Would it make sense to pass predicate with the options how shouldComponentUpdate check for update? I would want one selector with many properties that changes or not depending on some index, to connect in with different "Value" components. One Value component would display only one value - property but it would be connected to the same memoized selector.

something like:

export default connect(
  mapStateToProps,
  mapDispatchToProps,
  mergeProps,
  {
   shouldComponentUpdate: (props, nextProps) => props.someobj.intVal !== nextProps.someobj.intVal
  }
)(SomeComponent);

Cheers

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

@primozs I don't really understand your use case. When would you want a component that received changed props to not re-render?

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Depending on how #525 is resolved, we may need to just leave this alone, because it's possible the solution to that could require calling setState on every store update again. I don't think we can make a final decision on this until that one is resolved.

@timdorr

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Based on the findings in #525, let's put this on a 5.1 release and wait for React 16 to push that out (so users can easily clamp down to the 5.0 if they can't upgrade from React 15 easily).

@timdorr timdorr modified the milestones: 5.1, 5.0 Nov 8, 2016

@freddi301

This comment has been minimized.

Copy link

commented Dec 4, 2016

Why not just compose->replace shouldComponentUpdate?
If user defines its own it do not get shadowed but istead composed with react-redux one.
Maybe a chain of middleware (or just composed with logical ord) composed of sCU of various modules in the app (like some generalReactRouterShouldComponentUpdate which checks changes in context that uses)
executed like short-circuit logical or, could work.
Takeaway:
if component implements sCU -> get composed and replaced with react-redux sCU
react-redux offer an api to register application-wide sCU

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

It looks like the fix for #525 didn't affect whether we need to change our plans on this. So my opinion is we should just remove sCU completely... at least as the long term plan. The question being whether we offer a migration path in interim versions or not. Since removing sCU could have negative perf impact on someone relying on it to prevent unnecessary rerenders in their own components, I think we should treat its removal as a breaking change, which would mean it would land as version 6.0.

I suggest we:

  1. Land 5.0 with what we got.
  2. Give that a couple months for everyone to upgrade and shake out any issues.
  3. Land 6.0 with sCU removed, and release notes that offer alternatives (extend PureComponent or use one of recompose's several related HOCs.)
@markerikson

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

Just to recap and check my own understanding: connect implements sCU to handle the "props from any parent -> connected child" case as an extra perf benefit, right? So, no intrinsic requirement that sCU be there, just trying to cut down on re-rendering? And then the problem as described here is that that sCU interferes with context for other libs?

edit Boy, I should really go back and re-read the start of the thread. Yes, that's apparently the case.

@jimbolla

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

The 4.x implementation relied more heavily on sCU because of its order of operations when a store change is made. With the new implementation's avoidance of setState unless it's really necessary, that's no longer the case. Now, sCU will always be true if it's firing in response to a store change. It can only ever be false if it's happening because of receiving new props from parent.

simenbrekken pushed a commit to unfold/react-firebase that referenced this issue Dec 14, 2016

Simen Brekken
Remove pure option
Adhering with the updated react-redux API which [favors
composition](reduxjs/react-redux#507)

simenbrekken pushed a commit to unfold/react-redux-firebase that referenced this issue Dec 14, 2016

Simen Brekken
Remove pure option
Adhering with the updated react-redux API which [favors
composition](reduxjs/react-redux#507)
@timdorr

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Closing this out in favor of moving to the new context API.

@timdorr timdorr closed this Apr 20, 2018

@wilsonpage

This comment was marked as off-topic.

Copy link

commented Aug 17, 2018

Use pure: false option to bypass the shouldComponentUpdate() check within connect() HOC.

export default connect(mapStateToProps, null, null, {
  pure: false
})(TodoApp)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.