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.createContext() by @cellog #1000

Merged
merged 26 commits into from Nov 6, 2018

Conversation

9 participants
@cellog
Copy link
Contributor

cellog commented Aug 14, 2018

Supersedes #997, based off of master so no merge conflicts

takes into account @timdorr 's request regarding Provider.spec.js not using canary values
to see the commit history that led to this, check out #997.

This fixes
#914 (implements forwardRef and works with forwardRef element as connected component too)
#943 (removes prop-types in production build)
#802 (no try/catch block - mSP is called directly from render() and is bubbled up naturally)

and addresses
#950 (implements React context)
#890 (uses async-safe subscription to redux in Provider)

This PR combines an alternate approach to implementing Context to #995 and also the move to react-testing-library (#996) because testing React context with enzyme isn't yet possible. The PR does the following:

  • implements context and subscriptions via React's React.createContext()
  • adds support for refs directly via React.forwardRef
  • allows multiple children in the Provider component
  • allows changing the store prop in Provider (for hot reloading, or switching out for a new store)
  • allow nested Providers, the lowest provider in the tree controls children below it (feature of React context, not react-redux)
  • replaces passing store as prop by passing custom context provider to <Provider> and context consumer to connected components:
function Comp(props) {
  return <div>{props.hi}</div>
}

const Container = connect(...)(Comp)

const customContext = React.createContext()

function FancyPants() {
  return (
    <Provider store={store1}>
      Primary provider
      <Provider store={store2} context={customContext.Provider}>
        <Container>
          Uses store 1
          <Container>
            Also uses store 1
            <Container consumer={customContext.Consumer}>
              Uses store 2
              <Container /> <!-- this uses store 1 again -->
            </Container>
          </Container
        </Container>
      </Provider>
    </Provider>
  )
}
  • it reduces code size by almost 25% (drops 80 lines)
  • removing subscriptions greatly reduces maintenance complexity
  • no usage of getDerivedStateFromProps, instead, derived props are memoized in render()
  • more async-ready because all subscribing is done in componentDidMount and componentDidUpdate of the Provider component, subscriptions are not handled at all in connected components, except to read from React's context consumer. This doesn't address tearing and other async issues that are caused by redux being synchronous in nature. Fixing this will require breaking redux's backwards compatibility, and that's a different discussion.
  • probably more hot loader compatible, again because subscriptions are isolated in Provider and handled by React context.
  • Tests: removes testing in React prior to 16.4, ups dependencies to React 16.4+, adds react-testing-library dependency, removing enzyme and react-test-renderer deps. It preserves the multi-version testing framework, in order to add new React versions for testing as they are released. Several tests are updated to support the backwards-compatibility breaks

Drawbacks to this approach:

  • BC break: withRef is gone for connected components. The code errors out explaining that the user should simply pass a ref in and it will work. This will cause codebases to break that are relying on withRef. Fortunately, the fix is to remove withRef and update code to use standard reference handling procedures in React. docs need to be updated
  • BC break: store cannot be passed as a prop to connected components. The code errors if a user attempts to pass store to a connected component. The error explains to use a custom Provider (as noted above) to fix this, but will cause breaks in the few programs using this feature. docs need to be updated
  • BC break: storeKey is irrelevant in connect() options. The code errors if a user attempts to pass this option in connect()'s options. The error explains to simply use a nested Provider or a custom Provider. docs need to be updated
  • requires 3 wrapper components, Connect, the Context consumer, and PureComponent wrapper around the component in pure mode (2 in impure mode). This is what makes the simplicity possible. The top-most component retrieves the context containing redux state, calculates derived props, and passes them to the inner component. Rather than try to do all of this before shouldComponentUpdate, which leads to tons of complexity and brittle code, we pass it to the underlying component. sCU in PureComponent works perfectly for our needs, preventing update if the derivedProps are unchanged.
  • in order to support forwardRef, we have to import react-is, which does not yet have an esm build, so we gain a little heft in exchange for the reduction in code
  • the existing hot loading tests don't work at all, and had to be stripped. The implementation assumes that when hot loading, we may get passed a new store to Provider, which will require unsubscribing from the old and subscribing to the new in componentDidUpdate, but this is not tested in the real world yet. There are tests for this new behavior in Provider.spec.js
  • because tests didn't work at all, the diff is huge.

@markerikson @timdorr

Show resolved Hide resolved package.json
Show resolved Hide resolved src/components/Context.js Outdated
@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 18, 2018

I just ran another perf benchmark for 5.0.7, #995, and #1000 , set to 2500 connected components. Here's the FPS (avg / min-max) and Chrome trace time split results for each one (30-second test runs):

FPS Scripting Painting Rendering
5.0.7 28.26 (24-30) 17326 9569 2072
#995 24.26 (19-28) 19067 8246 1767
#1000 15.8 (14-17) 22401 5589 1187

The implementation of #1000 is conceptually interesting, and I like its relative simplicity (just memoize in render vs splitting across two components), but based on these numbers it seems noticeably slower.

Can anyone else confirm the numbers? I did fresh builds from both PR branches (including tweaking the Rollup config to set output.sourcemap : true, and updating the perf script to copy the sourcemaps to the app build folder to get some better names in the traces).

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Aug 18, 2018

can you link to the perf script/setup you're using again?

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Aug 18, 2018

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Aug 18, 2018

needs a pushed commit with your changes though @markerikson

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 18, 2018

I'm going to try updating #995 to add in forwardRef and stuff so that it's more equivalent to #1000 , then try re-running the comparison.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 18, 2018

I pushed the sourcemap usage changes to the perf repo. For building React-Redux, it's just adding sourcemap : true to the output {} object in rollup.config.js.

I'll throw the average calculation into the perf runner. It's probably not entirely correct, as the FPS value changes could be happening at different times, and so the "average" should really be calculated using frame timestamps or something, but it's at least an objective comparison.

Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved package.json Outdated

@cellog cellog referenced this pull request Aug 30, 2018

Merged

Benchmark suite #1008

@timdorr timdorr changed the title Use new Context, forwardRef for 6.x (alternate implementation to #995) Use React.createContext() by @cellog Sep 20, 2018

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Sep 20, 2018

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

npm install react-redux@next-1000
@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Sep 20, 2018

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

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Sep 20, 2018

I just pushed an update to this branch to use {forwardRef : true} instead of {withRef : "forwardRef"}, for consistency with #995 . The next-1000 tag has been republished (thanks, Tim!).

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 2, 2018

I'm looking at this again today, and I'm semi-stuck on the handling of forwardRef vs wrapperProps.

The current commit has totally alternate code paths for rendering the child component with and without a forward ref. There's near-duplicate renderWrappedComponent and renderWrappedComponentWithRef functions, and there's this bit of logic:

    if (forwardRef) {
      Connect.prototype.renderWrappedComponent = Connect.prototype.renderWrappedComponentWithRef

There's also alternate versions of a PureWrapper component for with and without passing down forwardRef.

I understand why that's there. It's because we want to maintain referential integrity of the wrapperProps object so that it can be easily checked for changes via a single === comparison in that memoization function:

        return (state, props, store) => {
          if ((connectOptions.pure && lastProps === props) && (lastState === state)) {
            return lastDerivedProps
          }

But, the code duplication really bothers me. And, 99.8% of the time people are going to be using standard connect, not connectAdvanced, and isn't that what the existing mapState-ish selector logic ultimately does anyway?

On the other hand, I could see that if we just went ahead and did <Connect {...wrapperProps} forwardRef={ref} /> in the forwarding function, and the ref was a callback ref, we could wind up with the combined "wrapper props" actually considered changed on every render. If mapState depends on ownProps, we'd be re-running that more often that actually necessary.

Any thoughts?

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Nov 2, 2018

Yes. The solution is for React to fix the performance drain. Until then, I don't yet see a better way to do this. We will be blamed for any performance hit, because of the redux bias out there.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 2, 2018

@cellog : what was the actual performance difference that you saw there?

@stevemao

This comment has been minimized.

Copy link
Contributor

stevemao commented Nov 3, 2018

I'm wondering if facebook/react#13739 is a blocker for perf. @alexreardon @gaearon thoughts?

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 3, 2018

I don't think it's an actual blocker, although yes, anything the React team can do to speed up context updates would be helpful. What I will say is that I want to start getting people to try this out in actual apps so that we can get feedback on real-world performance, not just the artificial stress-test benchmarks we've been trying to run.

@alexreardon

This comment has been minimized.

Copy link
Contributor

alexreardon commented Nov 3, 2018

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Nov 3, 2018

The action slowdown was about 45% in every benchmark. Most apps are not that intensive, so we could decide to offer the alternate code as a separate thing. Either way, I think we're stuck with 2 versions until React steps up

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Nov 3, 2018

oh and the issue was forwardRef, not context updates

@markerikson markerikson referenced this pull request Nov 5, 2018

Open

Provide React Hooks #1063

Rework connect tests, combine warnings, and remove observedBits
Ported connect test handling from 995
Deduped the "custom store context" messages
Removed use of observedBits for now
Commented out derivedProps propTypes that caused test failures
@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 6, 2018

I've been hacking on this tonight, and I've got something that at least passes the tests after a bunch of cleanup and restructuring. About to test this against the benchmarks repo.

markerikson added some commits Nov 6, 2018

Rework connect component based on review notes
Removed use of PureWrapper
Used React.memo() on the wrapped component
Renamed and extracted makeDerivedPropsSelector
Added makeChildElementSelector
Simplified render props callback
Simplified forwardRef handling
@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 6, 2018

Progress Update - Ready for Beta-ish?

I think I've got this branch not just cleaned up, but actually improved quite a bit. In particular, I've managed to simplify the logic around pure and forwardRef, and it looks like I've sped up this branch significantly compared to its earlier iterations. It's still not as fast as 5.0.7, but it's the fastest of the v6 variants we've tried thus far.

I think this is probably ready for a beta release of some kind so we can start getting real-world feedback.

Actually, one interesting side note. I'd originally said we ought to require React 16.6 as a minimum baseline, on the grounds that I was planning to use React.memo(). However, I wound up not using that. I guess that means we could probably get away with... 16.3, maybe? We're not using getDerivedStateFromProps, so we don't care about the behavior change there. The only "new" thing we're using is React.createContext(), and that's 16.3+. So, we could probably drop the minimum peerDep version back down to 16.3. I mean, if you're on 16.3, you can probably go to 16.6 without any problems, but might as well go for wider compat, I guess?

Performance Techniques

The big improvement in these latest commits was due to bringing back a trick from v4: memoizing the actual element reference returned from Connect.render(). It's not particularly documented, but if you return the exact same element reference from a render method as the last time, React will bail out of rendering that child. Since v4 did all of its real work in render(), the end looked like this:

        if (!haveMergedPropsChanged && renderedElement) {
          return renderedElement
        }

        if (withRef) {
          this.renderedElement = createElement(WrappedComponent, {
            ...this.mergedProps,
            ref: 'wrappedInstance'
          })
        } else {
          this.renderedElement = createElement(WrappedComponent,
            this.mergedProps
          )
        }

        return this.renderedElement

In v5, that kind of went away, since we do all the memoization work up front, and only call setState() once we know the wrapped component needs to re-render.

In v6, we're back to doing more work in render again thanks to the use of a <Context.Consumer>. Our previous attempts at handling the "no changes" case involved stuff like PureComponent as an extra wrapper for the real wrapped component.

I remembered the memoized element trick while playing with the hooks-ified branch I posted over in #1065 , and was able to reuse that with a handwritten memoization function here.

Performance Benchmarks

I've updated the benchmarks repo over at https://github.com/reduxjs/react-redux-benchmarks , and ran a new set of benchmarks with version 5.0.7, an earlier commit from this branch (4855c84, which I think was before Tim rebased it), and my new commit 2d06cdf .

The repo currently has three benchmark apps:

  • stockticker: a flat tree of N stock ticker components, rapidly updating at random
  • tree-view: a variation of the Redux tree-view example app, with N counter components in a seeded-randomized tree, and fake clicks on increment/delete/insert buttons at random
  • twitter-lite: three lists, with new entries continually inserting into them at random

They're artificial stress tests, but they are at least some kind of objective numbers that we can look at.

Each test is run for 30 seconds twice in Puppeteer. The first time tries to capture FPS results, the second activates a Chrome trace to capture scripting/rendering/painting times.

In theory, anyone ought to be able to clone the benchmarks repo, build from this branch, drop the UMD build into $BENCHMARKS_REPO/react-redux-versions, and roughly replicate these results.

And now, drumroll please, the results:

stockticker

+----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦
+------------------+---------+-----------+-----------+----------+
¦ 5.0.7            ¦ 24.32   ¦ 10325.44  ¦ 14866.93  ¦ 3261.55  ¦
+------------------+---------+-----------+-----------+----------+
¦ 6.0-1000-2d06cdf ¦ 18.40   ¦ 14596.15  ¦ 11756.86  ¦ 2474.33  ¦
+------------------+---------+-----------+-----------+----------+
¦ 6.0-1000-4855c84 ¦ 15.63   ¦ 17865.38  ¦ 9266.14   ¦ 1906.47  ¦
+----------------------------------------------------------------

tree-view

+----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦
+------------------+---------+-----------+-----------+----------+
¦ 5.0.7            ¦ 43.41   ¦ 9854.61   ¦ 10257.18  ¦ 704.06   ¦
+------------------+---------+-----------+-----------+----------+
¦ 6.0-1000-2d06cdf ¦ 42.37   ¦ 10476.60  ¦ 12690.35  ¦ 465.57   ¦
+------------------+---------+-----------+-----------+----------+
¦ 6.0-1000-4855c84 ¦ 41.78   ¦ 12019.21  ¦ 12074.83  ¦ 447.20   ¦
+----------------------------------------------------------------

twitter-lite

+-----------------------------------------------------------------
¦ Version          ¦ Avg FPS ¦ Scripting ¦ Rendering ¦ Painting ¦ 
+------------------+---------+-----------+-----------+----------+-
¦ 5.0.7            ¦ 40.24   ¦ 21362.26  ¦ 4813.54   ¦ 570.79   ¦ 
+------------------+---------+-----------+-----------+----------+-
¦ 6.0-1000-2d06cdf ¦ 31.90   ¦ 23361.35  ¦ 3659.78   ¦ 473.59   ¦ 
+------------------+---------+-----------+-----------+----------+-
¦ 6.0-1000-4855c84 ¦ 29.39   ¦ 24938.56  ¦ 2910.20   ¦ 419.44   ¦ 
+-----------------------------------------------------------------

Summary

All of our v6 PRs have been noticeably slower than v5 in these artificial benchmarks, and I don't think there's a lot we can do about that. v5 could bail out before React ever got involved, but with v6, React is always involved, and has to traverse the tree to find all of the potential context consumers.

However, this branch is measurably faster than our earlier attempts, and it's time we find out just how well it works in the real world.

Longer term, I'm hoping the React team will find ways to speed up context updates, which would benefit us considerably.

@timdorr, let's get this published as react-redux@next and see what happens! :)

timdorr added some commits Nov 6, 2018

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Nov 6, 2018

OK, ran things through Prettier (I'll get on top of #1071 after this) and we should be good to go.

@timdorr timdorr merged commit 85fb553 into reduxjs:master Nov 6, 2018

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
netlify/react-redux-docs/deploy-preview Deploy preview processing.
Details
security/snyk - package.json (Redux) No new issues
Details
security/snyk - package.json (timdorr) No new issues
Details
@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Nov 6, 2018

Yay!

@cellog

This comment has been minimized.

Copy link
Contributor

cellog commented Nov 8, 2018

It should warn if store is passed in props because we don't support that any more, and explain how to do it by passing in a context in the error message. Check the original tests from my PR to see how I did it if you want to copy

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