Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Use new Context, forwardRef for 6.x (alternate implementation to #995)#997

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

Closed
cellog wants to merge30 commits intoreduxjs:masterfromcellog:pre-6.x

Conversation

@cellog
Copy link
Contributor

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'sReact.createContext()
  • adds support for refs directly viaReact.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:
functionComp(props){return<div>{props.hi}</div>}constContainer=connect(...)(Comp)constcustomContext=React.createContext()functionFancyPants(){return(<Providerstore={store1}>      Primary provider<Providerstore={store2}context={customContext.Provider}><Container>          Uses store 1<Container>            Also uses store 1<Containerconsumer={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 ofgetDerivedStateFromProps, instead, derived props are memoized inrender()
  • more async-ready because all subscribing is done incomponentDidMount andcomponentDidUpdate of theProvider 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 inProvider 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 customProvider (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 inconnect() options. The code errors if a user attempts to pass this option inconnect()'s options. The error explains to simply use a nestedProvider or a customProvider. docs need to be updated
  • requires 3 wrapper components,Connect, the Context consumer, andPureComponent 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 beforeshouldComponentUpdate, which leads to tons of complexity and brittle code, we pass it to the underlying component. sCU inPureComponent works perfectly for our needs, preventing update if the derivedProps are unchanged.
  • in order to support forwardRef, we have to importreact-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 inProvider.spec.js
  • because tests didn't work at all, the diff is huge.

@markerikson@timdorr

This commitfixesreduxjs#965The essence of the problem is that getDerivedStateFromProps is called when the incoming props OR incoming local state changes. So we cannot store anything in state that is needed in shouldComponentUpdate.This commit splits up the tracking of incoming props, incoming store state changes, and removes getDerivedStateFromProps and the usage of local state to store any information.Instead, local state is used as a flag solely to track whether the incoming store state has changed.Since derived props are needed in shouldComponentUpdate, it is generated there and then compared to the previous version of derived props.If forceUpdate() is called, this bypasses sCU, and so a check in render() compares the props that should have been passed to sCU to those passed to render(). If they are different, it generates them just-in-time.To summarize:1) shouldComponentUpdate is ONLY used to process changes to incoming props2) runUpdater (renamed to triggerUpdateOnStoreStateChange) checks to see if the store state has changed, and stores the state, then updates the counter in local state in order to trigger a new sCU call to re-generate derived state.Because of these changes, getDerivedStateFromProps and the polyfill are both removed.All tests pass on my machine, but there is at least 1 side effects to the new design: - Many of the tests pass state unchanged to props, and pass this to child components. With these changes, the two updates are processed separately. Props changes are processed first, and then state changes are processed.I updated the affected tests to show that there are "in-between" states where the state and props are inconsistent and mapStateToProps is called with these changes. If the old behavior is desired, that would require another redesign, I suspect.
merge new rtl tests and test framework
passing store in props and hot update are only failing tests
this allows multiple concurrent react-redux subscriptions in the same component tree, using two root providers.By creating a custom context and passing that to the Provider and to connected components that use it, we get the full benefits of subscriptions with 2 different trees.
* remove hot reloading test, this is unnecessary now because Provider handles subscription through React context, and updates store subscription on componentDidUpdate* remove ProviderMock, it is just Provider now, in connect.spec.js* remove ReduxConsumer, Connect is now ReduxConsumer* error out if withRef is passed, users are expected to use forwardRef now* connect now allows passing in forwardRef() elements too through react-is
* createProvider is no longer necessary, because it is replaced by passing in context provider to Provider and consumer to connected components. Also, nested providers is natively supported in React
This refactor also lifts the "value" up into state for Provider, which is simpler. The bug was that componentDidUpdate was using nextProps instead of lastProps. Switching the state setting to use this.props was the correct behavior
@cellog
Copy link
ContributorAuthor

cellog commentedAug 13, 2018
edited
Loading

Forgot to mention the Issues this addresses/fixes

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)

@cellog
Copy link
ContributorAuthor

superseded by#1000

@cellogcellog closed thisAug 14, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cellog

[8]ページ先頭

©2009-2025 Movatter.jp