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

Merged
merged 26 commits into from
Nov 6, 2018
Merged

Conversation

cellog
Copy link
Contributor

@cellog 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

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

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

cellog commented Aug 18, 2018

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

@cellog
Copy link
Contributor Author

cellog commented Aug 18, 2018

@cellog
Copy link
Contributor Author

cellog commented Aug 18, 2018

needs a pushed commit with your changes though @markerikson

@markerikson
Copy link
Contributor

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

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.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@cellog cellog mentioned this pull request Aug 30, 2018
@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
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
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
Copy link
Contributor

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

Yay!

@cellog
Copy link
Contributor Author

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

albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
* alternate 6.x implementation

* use canary values in store instead of setting a prop on the store

* remove createProvider (missed this one before)

* fix renderCountProp, add a test

also clean up unused canary values

* remove errant console.log

* expose context consumer and provider

This will allow third party apps to use these in their code

* changes requested by @timdorr

* export Context instead of just Consumer/Provider
* fix error messages for removed functionality
* minor displayName change

* keep prop-types in production minified UMD build

when the time is right, one need only
change the BABEL_ENV for build:umd:min back to "rollup-production"

* performance optimizations: HEADS UP API change too

* React.forwardRef is VERY slow, on the order of 2x slower in our benchmark. So we only enable it if withRef="forwardRef" folks using withRef=true will get an error telling them to update and not rely on getWrappedInstance() but just to use the ref directly
* renderCountProp is removed, as this is natively supported in React dev tools now
* all usages of shallowEquals are removed for pure components, it was unnecessary.
* instead of allowing passing in a custom Context consumer in props, it is now required to be passed in via connect options at declaration time.

* small optimizations/refactors

* fix storeKey error, allow unstable_observedBits

* update hoist-non-react-statics

* cosmetics

* Replace `withRef="fowardRef"` option with `forwardRef=true`

* Less package-lock.json noise

* Bump React dep to 16.6, and remove shallow-equals

* Switch context variable and prop naming, and use the whole object

* Update Provider implementation and use storeState field

* Rework Provider tests

* 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

* 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

* Fix tests around custom context

* Fix custom context as a prop usage

* Fix lint warnings

* Run through Prettier. Update lockfile.
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.

9 participants