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

Use React Hooks for connect #1065

Closed
wants to merge 18 commits into from
Closed

Use React Hooks for connect #1065

wants to merge 18 commits into from

Conversation

markerikson
Copy link
Contributor

@markerikson 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.

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

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

alexreardon commented Oct 27, 2018

This change looks super impressive!!!

The connectAdvanced diff is incredible

@markerikson
Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u could avoid copying those props with

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

@mpeyper
Copy link
Contributor

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
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 😄

@timdorr
Copy link
Member

timdorr commented Mar 4, 2019

Based on discussion behind closed doors in the super-secret invite-only gatekeeper zone, i.e. Twitter DMs, we're going to close this one out and work on the hooks-based approach instead. I'll nuke the branch, but it will stick around in this PR if we want to pull from it.

@timdorr timdorr closed this Mar 4, 2019
@timdorr timdorr deleted the v6-experiment-hooks branch March 4, 2019 21:40
@alexreardon
Copy link
Contributor

Gasp!

@markerikson
Copy link
Contributor Author

Still sad that the v6 approach didn't work out how we thought, because the code in this branch was beautiful.

My v7 alpha branch? Yeah, that's... uh... "functional". That's a good word. It functions.

It ain't pretty, though.

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.

None yet

6 participants