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

implementation with useContextSelector (Experimental) ⚠️ #1350

Closed
wants to merge 1 commit into from

Conversation

@gnoff
Copy link
Contributor

@gnoff gnoff commented Jul 8, 2019

This PR implements the connect api for and hooks apis using a proposed RFC: Context Selectors

This is not an official React API

Goals

The point of this PR is to illustrate how libraries can shrink and become less complicated with this proposed API change. Please note not all tests pass because not all tests are relevant. It's possible there are edge case bugs too, this PR is for exploration and illustration, not for merging.

Claims

  • This implementation is almost as fast as v7, significantly faster than v6. It should be eminently usable
  • It supports concurrent mode correctly
  • It does not suffer zombie child problems, including with hooks use
  • It is significantly smaller
  • It is significantly simpler

How to use

get a working copy of react with useContextSelector

  1. clone gnoff/react#3 and checkout branch
  2. run yarn build (will take a while)
  3. run cd build/node_modules/react && npm link
  4. run cd ../react-dom && npm link

checkout this react-redux branch

  1. checkout gnoff-context-selectors
  2. run npm link react react-dom
  3. run tests to confirm things worked npx jest (some tests fail but you should get most passing)

Quick Access

You can see a built version of both the react and react-redux lib here: https://codesandbox.io/s/react-redux-context-selector-truop

gzip sizes

master: around 5.69kb
this PR: around 4.39kb

these sizes should be smaller in real bundles if dependency deduping is working correctly. this means that relative decrease in size should be magnified

@gnoff
Copy link
Contributor Author

@gnoff gnoff commented Jul 8, 2019

@markerikson here it is, properly described :)

Loading

@netlify
Copy link

@netlify netlify bot commented Jul 8, 2019

Deploy preview for react-redux-docs ready!

Built with commit bfaef05

https://deploy-preview-1350--react-redux-docs.netlify.com

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Jul 8, 2019

Thanks, I'll try to look at it later.

The immediate observation I'd make is that from a project management standpoint, this would effectively require another major, because:

  • We'd have to require React 16.whatevernnewnewcontext as a minimum peer dep
  • As we found out with v6, while it's not part of our public API, folks do currently rely on the "tearing" behavior where dispatched actions during mount are immediately visible in mounting child components. Reading a consistent state value from context actually breaks that expectation (especially important for SSR and dynamic reducer loading - see #1107 and #1150 ). Not saying it's a blocker, but if we do go for context-based state updates, we would need to consider that issue.

Loading

@brunolemos brunolemos mentioned this pull request Jul 8, 2019
@gnoff
Copy link
Contributor Author

@gnoff gnoff commented Jul 8, 2019

Thanks Mark,

Definitely would require a new major, no question there.

as for tearing, that's an interesting problem. Unfortunately I don't think there is a way for concurrent react support to exist and still have tearing support. On the flip side maybe when concurrent react is mature there will be a way to lean into it. Imagine yielding the render once a dispatch occurred so you could restart it with the updated state value. It would still be possible to get yourself into trouble but that true today too :)

The SSR bit is interesting, are you saying that because you need to synchronously render everything, mounted based dispatches need to be visible to children b/c there is no opportunity to wait for a second render to resolve things?

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 5, 2019

Hey @gnoff : out of curiosity, what are the perf numbers on this PR, vs v6 and v7?

Loading

@joshcstory
Copy link

@joshcstory joshcstory commented Nov 5, 2019

@markerikson oh man, i haven't looked at this in a few months. I'll see if I can do the benchmark app again and do a comparison. From what I recall it tracked just under v7 performance but close enough that you would consider them in the same performance category and it was miles ahead of v6.

When I ran it in fubo.tv (where I worked at the time) v6 had an update time of around 40ms for a relatively trivial update and 4ms for the same update using this PR's implementation

the perf gain here is amplified by the fact that the update didn't really change the UI at all so the cost overhead of v6 is amplified. But it does show that moving the bailout to be inside react rather than inside the connect HOC has significant performance implications

Loading

@markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 5, 2019

I ran into Sebastian at ReactConf and asked if he'd take a look at this. Dunno if he has yet.

Loading

@joshcstory
Copy link

@joshcstory joshcstory commented Nov 5, 2019

Heh, same, but no feedback yet

Loading

@timdorr
Copy link
Member

@timdorr timdorr commented Nov 22, 2019

Until the upstream proposal is accepted, there isn't really anything to do here. Thanks for looking into this, @gnoff!

Loading

@timdorr timdorr closed this Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants