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

Remove shouldComponentUpdate #625

Closed
wants to merge 2 commits into from
Closed

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Feb 17, 2017

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.

Let's see what happens...
@timdorr timdorr added this to the 5.1 milestone Feb 17, 2017
@markerikson
Copy link
Contributor

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

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

@gaearon
Copy link
Contributor

gaearon commented Feb 18, 2017

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

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

gaearon commented Feb 18, 2017

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

markerikson commented Feb 18, 2017

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

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

gaearon commented Feb 18, 2017

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

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

gaearon commented Apr 13, 2017

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

timdorr commented May 8, 2017

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

gaearon commented May 9, 2017

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

gaearon commented May 9, 2017

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

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

gaearon commented May 10, 2017

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

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

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

gaearon commented Nov 5, 2017

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

tyscorp commented Feb 8, 2018

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

@timdorr
Copy link
Member Author

timdorr commented Feb 9, 2018

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

@timdorr
Copy link
Member Author

timdorr commented Apr 20, 2018

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

@timdorr timdorr closed this Apr 20, 2018
@timdorr timdorr deleted the shouldntComponentUpdate branch April 20, 2018 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants