unimplement shouldComponentUpdate?

[jimbolla]

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]

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

[timdorr]

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]

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]

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]

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

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.

[levity]

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]

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

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]

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]

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

[timdorr]

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

[jimbolla]

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]

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]

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

[jimbolla]

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]

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

[freddi301]

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]

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]

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]

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.

[timdorr]

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