Use React.createContext() by @markerikson

[markerikson]

FOR DISCUSSION ONLY - DO NOT MERGE

This is a second attempt at the work done in #898 . Per #950 , the plan for React-Redux v6 is to use the new context API instead of the old deprecated context API.

This PR combines lessons learned from the work on a notional 5.1 PR in #980 , the first new context attempt in #898 , and a bunch more of me banging on things with a hammer until the tests shut up.

Note that this PR does not make use of React.forwardRef, and indeed I've just ripped out all of the withRef stuff for now. I'm just trying to see if the data flow works correctly. It also removes any ability to pass the store as a prop to a connected component. In addition, I also haven't tried out any potential perf optimizations around context and observedBits, either.

Summary of Changes

• As with React 16 experiment: rewrite React-Redux to use new context API #898 , <Provider> is now the only store subscriber. I've attempted to follow the pattern shown by Brian Vaughn in his create-subscription package.
• Like 898, I'm putting the entire Redux store state into context. Unlike 898, I'm also putting the entire actual store into context as well, rather than just dispatch. Long-term, I'm probably going to leave it this way so that other libraries could intercept the store and wrap it up with their own behavior (as discussed in Consolidated React 16.3+ compatibility and roadmap discussion #950 ).
• <Connect> has been split into two wrapper components. The outer one mostly just renders the <ReactReduxContext.Consumer> and the <ConnectInner> components. The real work is now done in <ConnectInner>, mostly in getDerivedStateFromProps.

I've really been wanting to avoid needing to use a second wrapper component, but the use of Context.Consumer pretty much mandates it due to the reliance on render props. I was originally planning on trying to use the new unstable_readContext() API (as described in facebook/react#13139 ) to stick with a single wrapper component, but Sebastian and Brian seem to think that's not a good idea.

This branch temporarily reverts all of our tests back to the older structure we had before the recent update for multiple React versions and use of Enzyme, due to Enzyme's inability to handle new context. (Yes, yes, Dan, I know you were making a point about that.) We're going to try converting the tests over to use react-testing-library instead, but I just wanted to see what I could get working before then.

We just merged in #998 , which converts our tests to use react-testing-library. I've re-created the code and test commits for this PR, and force-pushed them to this branch.

I've deleted a few tests that were obviously outdated (like making sure the store is in old context at the right key), and I'm also skipping some tests that either seem no longer relevant or aren't something I want to tackle right now:

'should unsubscribe before unmounting' 
'should recalculate the state and rebind the actions on hot update' 
'should use the store from the props instead of from the context if present' 
'should throw an error if the store is not in the props or context' 
'should throw when trying to access the wrapped instance if withRef is not specified' 
'should return the instance of the wrapped component for use in calling child methods' 
'should subscribe properly when a new store is provided via props' 

Other than that, the tests should be passing.

I've been trying to set up a perf benchmark harness over in #983 . I tried running v5.0.7 and this PR through that one stock ticker benchmark I've got set up.

Run with 1000 connected components, both versions stayed between 56-60 FPS. When I tried cranking it up to 2500 connected components, 5.0.7 dropped to between 20-27 FPS, while this PR was around 18-22. (For comparison, Jim's original benchmarks in #416 showed that v5 ran at 60 FPS with 330 components, while v4 choked down to single digits.)

In general, I would expect this PR to be a bit slower than v5 in some ways because of the overhead required in doing reconciliation for each new state, but faster in other because we're not immediately running all these mapState calls.

I fully expect this PR is still flawed in a bunch of ways, but it's a good starting point for further discussion based on lessons learned so far.

Obligatorily tagging @timdorr , @jimbolla , @cellog , @gaearon , @acdlite , @bvaughn, @sebmarkbage for thoughts.

[markerikson]

For funzies, here's zipped-up copies of Chrome perf trace captures from the 2500-component benchmark run:

Perf benchmarks - 5.0.7 vs PR 995.zip

Anyone ought to be able to replicate this by:

• Clone https://github.com/markerikson/benchmark-react-redux-perf
• Build this PR branch
• Name the prod UMD file react-redux-6.0-test2.min.js
• Copy it to the benchmark
• Set the benchmark's connected components constant to 2500 and rebuild
• Set the perf runner's versions to ["5.0.7", "6.0-test2"] and run it

[cellog]

Children.only is no longer required, no?

[markerikson]

Hmm. Y'know, I'm not entirely sure why we even ever had the requirement of a single child to begin with. Perhaps this is a very old holdover from the earliest prototypes of connect.

I don't immediately see a reason why we'd still need to enforce only one child.

[cellog]

this is not required with the new context any longer, we can change the context value and children are updated

[markerikson]

But, the selector initialization and binding of action creators happens when the connected children are instantiated - we don't try to re-bind those if the store is swapped out.

[cellog]

We can rebind in cDU, 1 line change

[markerikson]

Not something I'm particularly worried about right now. There may be someone out there who has a need to swap out their store, but it's not something we've ever supported and I don't see a need to try to support it for the time being.

[cellog]

here, I would rather do the calculation of derived props and memoize them, then the inner component need only be the WrappedComponent or a PureComponent that wraps it, something like:

const Inner = connectOptions.pure ?
  class Inner extends PureComponent {
    render() {
      return <WrappedComponent {...this.props} />
    }
  } : WrappedComponent

then renderInner can be something like:

  renderInner({ storeState, store }) {
    const props = this.props
    const derivedProps = this.memoizedProps(storeState, props)
    return <Inner {...props} {...derivedProps} />
  }

Our memoizer can be initialized in the constructor with this.memoizedProps = this.makeMemoizer()

  makeMemoizer() {
    let lastProps
    let lastState
    let lastDerivedProps
    let called = false
    return (state, props) => {
      if (called) {
        const sameProps = connectOptions.pure && shallowEqual(lastProps, props)
        const sameState = lastState === state
        if (sameProps && sameState) {
          return lastDerivedProps
        }
      }
      called = true
      lastProps = props
      lastState = state
      const nextProps = this.state.childPropsSelector(state, props)
      if (shallowEqual(lastDerivedProps, nextProps)) {
        return lastDerivedProps
      }
      lastDerivedProps = nextProps
      return lastDerivedProps
    }
  }

In this way, we don't need getDerivedStateFromProps at all

[markerikson]

Let's try that approach as a second PR, for comparison purposes.

[cellog]

I still think we can manage this, but by allowing passing a separate context consumer to connect. This way, users can use a custom context provider and just hook into it with their specific components. In other words, we provide createProviderContext and allow:

const { SpecialProvider, SpecialConsumer } = createProviderContext()
const ConnectedNormal = connect(...)(Thing)
const ConnectedOther = connect(mSP, mDP, merge, { contextConsumer: SpecialConsumer })(Thing)

function Something() {
  return (
    <Provider store={store}>
      <Provider context={SpecialProvider} store={store2}>
        <ConnectedNormal>
          <ConnectedOther />
        </ConnectedNormal>
      </Provider>
    </Provider>
  )
}

This way, we get the full advantage of React's context, but have flexibility.

[markerikson]

We could potentially allow passing a Context.Consumer directly to a connected component as a prop, although we might have to do some fiddling around because this currently tries to pass all the wrapper props down directly to the inner component.

That said, I'm not worried about trying to implement store-as-a-prop or anything like that atm. My main concern is getting the data flow and update behavior working right - we can add everything else after that.

[markerikson]

@cellog , @timdorr : I had a chance to show this PR and #1000 to @bvaughn earlier today. He's not familiar with the guts of React-Redux, but he didn't see any obvious flaws or problems with either approach. He did think that implementing most of the logic with memoization in render (the approach in #1000 ) might be a bit simpler and easier to maintain, but otherwise nothing glaringly wrong either way.

[markerikson]

I updated our benchmarks repo tonight, updated this #995 branch to make forwardRef optional, and then rebuilt from #995 and #1000 and ran the benchmarks. Current results:

Results for benchmark stockticker:
+-----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦ 
¦ 5.0.7            ¦ 15.95   ¦ 16549.43  ¦ 10420.52  ¦ 2045.64  ¦ 
¦ 6.0-1000-4855c84 ¦ 14.90   ¦ 18243.08  ¦ 8895.77   ¦ 1741.93  ¦ 
¦ 6.0-995-9210282  ¦ 12.76   ¦ 20039.79  ¦ 7729.26   ¦ 1503.05  ¦ 
+-----------------------------------------------------------------

Results for benchmark tree-view:
+----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦
¦ 5.0.7            ¦ 44.00   ¦ 9627.22   ¦ 9947.20   ¦ 662.78   ¦
¦ 6.0-1000-4855c84 ¦ 42.39   ¦ 13272.30  ¦ 7828.64   ¦ 526.37   ¦
¦ 6.0-995-9210282  ¦ 41.71   ¦ 14573.84  ¦ 7497.27   ¦ 500.23   ¦
+----------------------------------------------------------------


Results for benchmark twitter-lite:
+----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦
¦ 5.0.7            ¦ 57.86   ¦ 21355.14  ¦ 6291.97   ¦ 720.06   ¦
¦ 6.0-1000-4855c84 ¦ 56.35   ¦ 22500.03  ¦ 5506.43   ¦ 679.90   ¦
¦ 6.0-995-9210282  ¦ 53.26   ¦ 23057.37  ¦ 5031.57   ¦ 654.91   ¦
+----------------------------------------------------------------

So, what I'm seeing is that both #995 and #1000 are within shouting distance of 5.0.7 in stress-test scenarios. 1000 is a bit slower than 5.0.7, and 995 is a bit further back. The bad news is that both seem to be spending noticeably more time "scripting". To be honest, I'm not sure we can do anything about that. 5.0.7 can bail out early because it runs its memoized selectors directly in the subscription callbacks and quit before it actually asks React to update. Both 6.0 branches are reliant on React updating the component tree and propagating the new state through context.

I'd like us to do some investigation into where these two implementations are spending their time, and see if we can do any further perf optimizations other than the observedBits stuff. After that, hopefully we can come to some kind of conclusion on which approach we want to go with for v6.

[cellog]

Why did you disable the context observedBits stuff in the benchmark? That was important for performance, perhaps the single biggest enhancement

[markerikson]

Right, that's exactly why I disabled them. I wanted to see how the two branches currently perform without any observedBits behavior. Also, for now I want us to focus on any additional optimizations we can do without observedBits, and then come back to it later.

If you want to create a branch in the perf repo that has the context usage in the apps so we can keep them around, go ahead.

[timdorr]

The latest version of this PR is now published to npm under the next-995 tag: https://www.npmjs.com/package/react-redux?activeTab=versions

npm install react-redux@next-995

[timdorr]

Oh, and renamed this and #1000 so they pair up well in the PR list and browser's tab bar.

[timdorr]

Given that #1000 is slightly faster and appears to have more finalized code (no TODOs), I'm going to close this out so we've only got one PR to think through. @cellog has granted us edit permissions on his PR branch, so we can focus any development efforts there. @markerikson, if you wanted to bring over any code to that PR, feel free.