Remove shouldComponentUpdate

[timdorr]

Fixes #507 and like every context subscription-related bug ever.

This is just a spike of me editing on the web interface (not on a machine with Node installed...). Travis builds will fail, things will break, no one will be happy. But it gets the ball rolling on removing it, so it's a first step.

[markerikson]

Whoa. That is some seriously advanced hacker-fu there. bows down

So just to confirm my own understanding: now that we're doing everything with layers of memoized selectors, sCU is basically pointless because we're doing the same kind of work internally, and keeping the wrapped component from re-rendering unless something actually changed?

[jimbolla]

@markerikson Yeah. I think #507 (comment) explains why sCU isn't a requirement anymore.

[gaearon]

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:

That would be a breaking change, right? Many apps are likely relying on this behavior—if not for functionality then for performance. I agree it might be better not to impose purity on everyone, but people are already treating connect() as a way to give purity, and breaking that promise should only be done in a major IMO.

[markerikson]

Yeah, it would probably be worth a major bump. I guess what I'm trying to nail down is: does the memoized internals approach give the effect of sCU, without actually needing to have it implemented at the React level? if it's removed and the wrapper component is asked to re-render with the same values, what happens? Or is sCU still necessary to keep things from rippling downwards?

[gaearon]

I can’t answer this because I don’t know how the new implementation works.
I’d be happy to answer any questions about React (or the old implementation) though!

[markerikson]

Yeah, that was really meant for @jimbolla .

Looking at that deleted couple lines, I'm pretty sure sCU is still necessary for preventing re-renders from parents, and that without it, the wrapper is really only able to deal with changes from map*-related values.

Although re-reading Jim's comment from the other thread, maybe I'm misunderstanding this?

[jimbolla]

This would certainly be a breaking change. sCU is no longer needed for changes coming from store, but still affects changes coming from props. People, including myself, rely on connect() as a convenient way to get the same benefits as PureComponent.

I'm feeling like this should wait until "React 16 - Now With FIber™" lands so that we can see how it affects performance. It's possible that R16 could change the way we get maximum performance and we end up wanting to keep sCU.

[gaearon]

I would add that after React 16 (e.g. maybe in 17) we'll likely make Fiber async by default (you can flip the switch here if you build it yourself). As a result, setStates would be batched in the correct order by default, and I’m not sure the complexity of notifying the listeners in the right order will be as important. Also, setState callbacks (which, as I understand, we’re using to ensure the correct order) have performance implications in Fiber and prevent some optimizations so it would be nice to not rely on them.

[markerikson]

Ah... as far as I know, we don't use setState for ordering. Jim wrote a nested subscription system - children effectively subscribe to their nearest connected ancestor if one exists, so that they're only notified after the ancestor is notified.

[gaearon]

Heads up: React 16 might not need this but it is possible React will rely more on sCU in the future in order to better reuse work in async scenarios. The details are still very fuzzy (we just started looking into trying to use async rendering at FB) but I wouldn’t hurry with this change in case we need to backtrack later. The context API is another beast because that’ll need a redesign anyway (likely in 17).

[timdorr]

OK, finally updated this damn thing.

I removed some tests related to f7dc41d, which was some of the original attempts at optimizations. This raises a slightly modified question: Should we be responsible for handling sCU?

The whole point was to be automatically aware of state changes that don't require updates and stop them with sCU. The unfortunate side effect was this required us to handle ownProps updates as well and we ended up applying our update-blocking logic to those props.

But I think this may be a mistake. We're making assumptions about how we want connected components to update, while lacking actual knowledge of the business meaning of ownProps. I think implementing sCU should be left up to the user. Very commonly, they end up getting lazy and not ever doing it (I know I'm guilty of this!), even in non-connected components. We're basically saying to skip the optimization in development because React Redux will take care of it for you. But meanwhile, we don't know enough to make an intelligent decision about whether or not to update. Shallow comparisons may be insufficient, deep comparisons may negate the performance benefit. We just don't know.

So, this may involve more than just fixing some code. We would have to be sure to educate people about this. And it may be a little painful, but I think it's worthwhile to get people thinking about writing their own sCU's.

[gaearon]

I am very concerned about changing something like this, especially after we already have a report about 5.x significantly regressing on performance. #686

[gaearon]

Basically, the way I think about it that Redux plainly wouldn't work on larger apps unless you aggressively optimize sCU. It makes sense to me to do this by default, as this is a more opinionated way to use React. Just like Om.

[Pensarfeo]

Does this really solve the problem? or simply shifts the responsibility to the users, like me :)
I think the issue of removing sCU goes into a more fundamental questions: what is the purpose of this library? I personally also use this library to control the update of my components according to the origin of data being passed to my components: either data from the store or props from the parent component.
If you remove the sCU we would not only push us to refactor lots and lots of code, but it would also make it very annoying to do so since we wont have a way to easily differentiate where are the props coming from.
Why not adding an option to update the component on context change too? Or using a library like the one described by markerikson above?

[gaearon]

Also, I don’t think this will fix context bugs in practice. IMO you need to use sCU in a Redux app (otherwise single store is impractical), and as soon as you implement your first sCU, context is broken again.

[wtgtybhertgeghgtwtg]

I'm feeling like this should wait until "React 16 - Now With FIber™" lands so that we can see how it affects performance. It's possible that R16 could change the way we get maximum performance and we end up wanting to keep sCU.

So, now that react@16 has landed, will this get another look?

[markerikson]

@wtgtybhertgeghgtwtg : maybe. Dan and Andrew have both said that the async React behavior they're currently prototyping and experimenting with will likely require changes in how other libs interact with React. Andrew has also said that at some point he'd like to have a discussion with the various state management lib maintainers on this topic.

React 16.0 deliberately maintains as much backwards compatibility with 15.x as possible. Later iterations of 16 may start changing that.

So, it's still too early to know what sorts of changes React itself is going to have and how those will affect the ecosystem, and therefore too early to know what's going to need to change with React-Redux.

[gaearon]

So, it's still too early to know what sorts of changes React itself is going to have and how those will affect the ecosystem, and therefore too early to know what's going to need to change with React-Redux.

👍

Let's hold off on any big changes until next summer. I think a lot will be clearer by then.

[tyscorp]

Will the new context API (facebook/react#11818) mean that this change will no longer be neccessary?

[timdorr]

@tyscorp Only for libraries blocked by connect() that use it. It doesn't automatically apply to older libraries.

[timdorr]

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