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

Use React Hooks for `connect` #1065

Open
wants to merge 18 commits into
base: master
from

Conversation

6 participants
@markerikson
Copy link
Contributor

markerikson commented Oct 26, 2018

This PR builds on the previous React-Redux version 6 preview work from #898, #995, aand #1000 . As with the previous couple PRs, the major changes are:

  • Switching from legacy context to createContext()
  • Switching from withRef to forwardRef
  • Removing the ability to pass a Redux store as a prop to a connected component

Our existing v6 PRs both passed our unit test suite, but the internal implementations were ugly. Using createContext required that we split the wrapper component into two pieces: an outer wrapper to render the context consumer, and an inner wrapper to receive the store state, process it, and render the real wrapped component.

With the announcement of React Hooks, there's now another way to access the store state from context: via the useContext() hook.

I've taken my existing branch from #995, and rewritten the wrapper component to be a function component that uses hooks. The implementation is dramatically simpler. v5, #995, and #1000 all have around 140 lines of complicated class-based component logic. In this PR, the wrapper component is simply:

    function createChildSelector(store) {
      return selectorFactory(store.dispatch, selectorFactoryOptions)
    }

    function ConnectFunction(props) {
      const {context, forwardRef, ...wrapperProps} = props
      const contextToRead = context || defaultContext

      const {store, storeState} = useContext(contextToRead)
      const childPropsSelector = useMemo(() =>  createChildSelector(store), [store])

      const childProps = childPropsSelector(storeState, wrapperProps)

      const renderedChild = useMemo(() => {
        return <WrappedComponent {...childProps} ref={forwardRef} />
      }, [childProps, forwardRef])

      return renderedChild
    }

I ran a test build through our benchmark suite just to see how it compares. While this build was still slower than v5, particularly in the stockticker artificial stress test, it's slightly faster than either of the existing v6 PRs.

I think this PR completely supersedes both #995 and #1000 , and is our best candidate approach for React-Redux v6.

It does rely on an alpha build of React (currently 16.7.0-alpha.0 ), and so we would have to leave it as an alpha in turn until the Hooks API is finalized and React 16.7 has gone final.

I'd like to publish this as a new alpha tag on NPM, and get people to start playing with it so that we can find out how it performs in real applications.

markerikson added some commits Aug 11, 2018

Add missing functionality for connect and Provider
Added ability to swap stores
Removed single-child limitation

Added invariant warnings for storeKey and withRef
Added valid element check using react-is
Refactored child selector creation for reusability
Added prop types for components
Added forwardRef and consumer as prop capability
Added a tiny memoized function for wrapper props handling
Removed semicolons
Rewrite connectAdvanced to use hooks internally
Replaced multiple levels of class wrappers with a simple hook that
reads the store state from context, initializes and uses the
provided selector, and memoizes the wrapped component as needed.

Updated Provider and connect to accept an entire context object
as a prop, because `useContext` only works with a context object
and not just the consumer.

Also allows passing a new store instance to Provider, because we
can easily reset the subscription aspect now.

Updated tests.
@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Oct 27, 2018

one unresolved question to ask the React team is how/whether observedBit will become part of context, because that resulted in a 2x speedup to #1000 in the stockticker benchmark. This looks very promising!

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Oct 27, 2018

@cellog : I did actually have the opportunity to talk to most of the React team today (Andrew, Sebastian, Dan, and Sophie). They were nice enough to listen to my questions and respond. I'm not going to say I entirely got answers for everything, but we did talk.

Unfortunately, the day has also been a blur, and I don't remember most of the details at this point. Sorry :( Maybe @gaearon , @sophiebits, or @acdlite can pop in and remind me what the rough plans are there.

There's really two related issues around performance here: how fast React can traverse the tree and find consumers (per facebook/react#13739 ), and then React potentially skipping running updates for consumers based on observedBits (see #1018 for notional ideas).

I think we ought to focus on just getting v6 ready as-is for now, and let the observedBits stuff kinda sit in the background for later.

I'll also note that I'm not entirely sure just how useful the stockticker benchmark actually is, on the grounds that A) it's a completely flat component tree, and B) the speed of the updates. It's certainly an objective number we can look at, I just don't know how meaningful the results are.

@alexreardon

This comment has been minimized.

Copy link
Contributor

alexreardon commented Oct 27, 2018

This change looks super impressive!!!

The connectAdvanced diff is incredible

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Oct 27, 2018

Hey, @timdorr : we need to figure out our branching strategy for v5 / v6. Thoughts?

const childProps = childPropsSelector(storeState, wrapperProps)

const renderedChild = useMemo(() => {
return <WrappedComponent {...childProps} ref={forwardRef} />

This comment has been minimized.

@Andarist

Andarist Oct 27, 2018

Contributor

u could avoid copying those props with

const renderedChild = useMemo(() => {
  const finalProps = forwardRef ? {...childProps, ref: forwardRef} : childProps
  return <WrappedComponent {...finalProps} />
}, [childProps, forwardRef])
@mpeyper

This comment has been minimized.

Copy link

mpeyper commented Oct 27, 2018

I'm on my phone, so it's a bit hard to see, but it looks like the only way to intercept the store itself is to provide your own context to Provider? That would mean all connected components would also need to be passed the same context, right?

Would there be anything preventing 2 providers to exist using different contexts (one for a library to consume and another default one for connected components to use)?

I'm thinking that the library could export its own Provider component that sets up the 2 react-redux providers and then consumes its own context to get the store and can pass on its modified store down the regular context. Does that sound feasible?

@timdorr timdorr changed the title v6 Candidate Preview: Use React Hooks for `connect` Use React Hooks for `connect` Oct 28, 2018

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Oct 28, 2018

Removing the prefix from the PR title. Versioning-wise, I don't think this makes sense as a 6.0. Depending on something that might not be out for a while will just delay us even more. I'd rather get off the pot on #995/#1000 and just ship something there as 6.0. We can iterate from there. A few different majors in a short period of time isn't something to be afraid of.

Side note/nit pick: We really need to get Prettier enabled on this repo too. The formatting is killing my OCD in some places 😄

@Andarist Andarist referenced this pull request Oct 28, 2018

Open

[WIP] Use Hooks #967

0 of 3 tasks complete

@NMinhNguyen NMinhNguyen referenced this pull request Nov 4, 2018

Merged

Use Prettier #1071

@nasreddineskandrani nasreddineskandrani referenced this pull request Nov 5, 2018

Merged

RFC: React Hooks #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment