Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.3k
Use React.createContext() by @markerikson#995
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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:
|
src/components/Provider.js Outdated
| returnChildren.only(this.props.children) | ||
| return( | ||
| <ReactReduxContext.Providervalue={this.state}> | ||
| {Children.only(this.props.children)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Children.only is no longer required, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 avery old holdover from the earliest prototypes ofconnect.
I don't immediately see a reason why we'd still need to enforce only one child.
src/components/Provider.js Outdated
| if(this[storeKey]!==this.props.store){ | ||
| Provider.getDerivedStateFromProps=function(props,state){ | ||
| if(state.store!==props.store){ | ||
| warnAboutReceivingStore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not required with the new context any longer, we can change the context value and children are updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We can rebind in cDU, 1 line change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not something I'm particularly worried about right now. There may besomeone 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.
src/components/connectAdvanced.js Outdated
| */ | ||
| renderInner(providerValue){ | ||
| const{storeState, store}=providerValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
constInner=connectOptions.pure ?classInnerextendsPureComponent{render(){return<WrappedComponent{...this.props}/>}} :WrappedComponent
then renderInner can be something like:
renderInner({ storeState, store}){constprops=this.propsconstderivedProps=this.memoizedProps(storeState,props)return<Inner{...props}{...derivedProps}/>}
Our memoizer can be initialized in the constructor withthis.memoizedProps = this.makeMemoizer()
makeMemoizer(){letlastPropsletlastStateletlastDerivedPropsletcalled=falsereturn(state,props)=>{if(called){constsameProps=connectOptions.pure&&shallowEqual(lastProps,props)constsameState=lastState===stateif(sameProps&&sameState){returnlastDerivedProps}}called=truelastProps=propslastState=stateconstnextProps=this.state.childPropsSelector(state,props)if(shallowEqual(lastDerivedProps,nextProps)){returnlastDerivedProps}lastDerivedProps=nextPropsreturnlastDerivedProps}}
In this way, we don't needgetDerivedStateFromProps at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's try that approach as a second PR, for comparison purposes.
| Connect.getDerivedStateFromProps=getDerivedStateFromProps | ||
| // TODO We're losing the ability to add a store as a prop. Not sure there's anything we can do about that. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 providecreateProviderContext and allow:
const{ SpecialProvider, SpecialConsumer}=createProviderContext()constConnectedNormal=connect(...)(Thing)constConnectedOther=connect(mSP,mDP,merge,{contextConsumer:SpecialConsumer})(Thing)functionSomething(){return(<Providerstore={store}><Providercontext={SpecialProvider}store={store2}><ConnectedNormal><ConnectedOther/></ConnectedNormal></Provider></Provider>)}
This way, we get the full advantage of React's context, but have flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could potentially allow passing aContext.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.
@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 |
Added ability to swap storesRemoved single-child limitationAdded invariant warnings for storeKey and withRefAdded valid element check using react-isRefactored child selector creation for reusabilityAdded prop types for componentsAdded forwardRef and consumer as prop capabilityAdded a tiny memoized function for wrapper props handlingRemoved semicolons
I updated our benchmarks repo tonight, updated this#995 branch to make 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 optimizationsother than the |
Why did you disable the context observedBits stuff in the benchmark? That was important for performance, perhaps the single biggest enhancement |
Right, that's exactly why I disabled them. I wanted to see how the two branches currently perform without any 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. |
The latest version of this PR is now published to npm under the npm install react-redux@next-995 |
Oh, and renamed this and#1000 so they pair up well in the PR list and browser's tab bar. |
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. |
Uh oh!
There was an error while loading.Please reload this page.
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 doesnot make use of
React.forwardRef, and indeed I've just ripped out all of thewithRefstuff 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 andobservedBits, either.Summary of Changes
<Provider>is now the only store subscriber. I've attempted to follow the pattern shown by Brian Vaughn in hiscreate-subscriptionpackage.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 inConsolidated 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 ingetDerivedStateFromProps.I've really been wanting to avoid needing to use a second wrapper component, but the use of
Context.Consumerpretty much mandates it due to the reliance on render props. I was originally planning on trying to use the newunstable_readContext()API (as described infacebook/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 usereact-testing-libraryinstead, 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:
Other than that, the testsshould 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
mapStatecalls.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.