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

Experiment/POC: Optimize for use with React 16. #856

Closed
wants to merge 1 commit into from
Closed

Experiment/POC: Optimize for use with React 16. #856

wants to merge 1 commit into from

Conversation

jimbolla
Copy link
Contributor

@jimbolla jimbolla commented Jan 18, 2018

DISCLAIMER: I haven't perf tested this at all yet, but I felt like it was time to get the ball rolling on this to start the discussion.

So in React Redux 5, one of the key design points was that in React 14/15, calls to setState were expensive and to be avoided when possible. Props calculation was done before calling setState. To ensure client components always got up-to-date props from parent components, the Subscription concept was needed to ensure 2 invariants:

  1. Connected parent components should always notified before child components.
  2. Connected parents should always pass their latest props to their child components before the child components attempt to recompute their own new props against stale props.

This added complexity to the code; and improved performance in most use cases but not all.

React 16 made some significant changes to the way setState's underlying system works, as well as added an important feature that I think React Redux can utilize (the ability to return null in the updater func to cancel the setState). So instead of avoiding calls to setState, this version completely relies on it. This significantly simplifies the code and (hopefully) improves performance by relying on React's own optimizations.

Key change:

Subscriptions are no longer a thing and have been removed from Provider & connectAdvanced. Invariant 1 is now satisfied by having child components telling their parents to subscribe first (via the subscribeFirst context value). Invariant 2 is now satisfied by moving props computation inside the setState call and relying on React Fiber to call it at the appropriate time. It also relies on the new feature of setState of being able bail out by returning null.

The hypothesis is that calling setState a lot but being able to bail out early will be less expensive overall than the blocking that the old algorithm utilized.

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2018

cc @acdlite

@timdorr
Copy link
Member

timdorr commented Jan 18, 2018

So, a good question here is: Does this even function under React <=15? Could we support both, even if it was non-performant?

@jimbolla
Copy link
Contributor Author

@timdorr I'm not sure how React 15 behaves if one does setState(() => null). If it doesn't explode, I'd still expect a lot of broken tests and terrible performance.

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2018

Any harm in cutting 6.x that's 16+?

@timdorr
Copy link
Member

timdorr commented Jan 18, 2018

Not that I can think of. As long as we do like React Router and keep 5.x/4.x under maintenance to support those that can't upgrade easily.

@jimbolla
Copy link
Contributor Author

@gaearon @timdorr That seems feasible and probably the simplest path forward.

@timdorr
Copy link
Member

timdorr commented Jan 25, 2018

I think we may want to get on top of facebook/react#11818 at the same time, if we're going to go 16-only. That would be a separate change, but might also significantly overhaul our implementation. I'll dive into this soon, but it's something to keep in mind.

@ematipico
Copy link

Moving to 16 and using the new context API means that the minimal version installed of React has be greater than 16.3, unless React Redux will be able to determine if the new API is available or not (and falling back to the current usage of the context API)

@timdorr
Copy link
Member

timdorr commented Feb 2, 2018

@ematipico I'm currently working through this issue on React Router. But again, it's a separate issue. This PR is only dependant on React 16.0.0 or higher.

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2018

@ematipico this shouldnt be much of a problem, peerDeps can be specified more granularly, are not tied to major versions, so a library should be able to just do "react": "^16.3.0"

@Cryrivers
Copy link

I guess it would be better if it can utilize the new Context API provided by React 16.3

@timdorr
Copy link
Member

timdorr commented Mar 6, 2018

I guess it would be better if it can utilize the new Context API provided by React 16.3

That's an ancillary concern. That work can proceed independent of the work here.

@markerikson
Copy link
Contributor

This PR has been superseded by the discussions and changes in #950, #995, and #1000 , so I'm closing it.

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

Successfully merging this pull request may close these issues.

7 participants