-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Remove usages of async-unsafe lifecycle methods #919
Changes from 6 commits
7b4e162
6ad378b
20de413
94f0b8a
9ee1a36
7344f3d
a7ba298
27b2d4b
48c8a75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import hoistStatics from 'hoist-non-react-statics' | ||
import invariant from 'invariant' | ||
import { Component, createElement } from 'react' | ||
import polyfill from 'react-lifecycles-compat' | ||
|
||
import Subscription from '../utils/Subscription' | ||
import { storeShape, subscriptionShape } from '../utils/PropTypes' | ||
|
@@ -23,6 +24,10 @@ function makeSelectorStateful(sourceSelector, store) { | |
selector.shouldComponentUpdate = true | ||
selector.error = error | ||
} | ||
}, | ||
clean: function cleanComponentSelector() { | ||
selector.run = noop | ||
selector.shouldComponentUpdate = false | ||
} | ||
} | ||
|
||
|
@@ -86,6 +91,11 @@ export default function connectAdvanced( | |
[subscriptionKey]: subscriptionShape, | ||
} | ||
|
||
function getDerivedStateFromProps(nextProps, prevState) { | ||
prevState.selector.run(nextProps) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks mutative. What does it actually do? I'm worried that this won't be safe and then avoiding the warning just gives a false sense of security. The method signature is intentionally "get" to indicate it should be pure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not pure but it's idempotent: when you run in twice, it has the same effect as if it was just once. I can try to make it pure by moving all the fields from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's try that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did that. It required some minor changes in tests: a7ba298#diff-b5fd7a42ea1b8efa382416b4a323003c It's quite clear why |
||
return null | ||
} | ||
|
||
return function wrapWithConnect(WrappedComponent) { | ||
invariant( | ||
typeof WrappedComponent == 'function', | ||
|
@@ -117,7 +127,6 @@ export default function connectAdvanced( | |
super(props, context) | ||
|
||
this.version = version | ||
this.state = {} | ||
this.renderCount = 0 | ||
this.store = props[storeKey] || context[storeKey] | ||
this.propsMode = Boolean(props[storeKey]) | ||
|
@@ -129,7 +138,9 @@ export default function connectAdvanced( | |
`or explicitly pass "${storeKey}" as a prop to "${displayName}".` | ||
) | ||
|
||
this.initSelector() | ||
this.state = { | ||
selector: this.createSelector() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked at what StrictMode warns about yet. Does it complain about instance variables? Is that why we're putting the selector into state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it doesn't. This is because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh, I see it now. Got it. |
||
} | ||
this.initSubscription() | ||
} | ||
|
||
|
@@ -152,25 +163,20 @@ export default function connectAdvanced( | |
// dispatching an action in its componentWillMount, we have to re-run the select and maybe | ||
// re-render. | ||
this.subscription.trySubscribe() | ||
this.selector.run(this.props) | ||
if (this.selector.shouldComponentUpdate) this.forceUpdate() | ||
} | ||
|
||
componentWillReceiveProps(nextProps) { | ||
this.selector.run(nextProps) | ||
this.state.selector.run(this.props) | ||
if (this.state.selector.shouldComponentUpdate) this.forceUpdate() | ||
} | ||
|
||
shouldComponentUpdate() { | ||
return this.selector.shouldComponentUpdate | ||
shouldComponentUpdate(nextProps, nextState) { | ||
return nextState.selector.shouldComponentUpdate | ||
} | ||
|
||
componentWillUnmount() { | ||
if (this.subscription) this.subscription.tryUnsubscribe() | ||
this.subscription = null | ||
this.notifyNestedSubs = noop | ||
this.store = null | ||
this.selector.run = noop | ||
this.selector.shouldComponentUpdate = false | ||
this.state.selector.clean() | ||
} | ||
|
||
getWrappedInstance() { | ||
|
@@ -185,10 +191,9 @@ export default function connectAdvanced( | |
this.wrappedInstance = ref | ||
} | ||
|
||
initSelector() { | ||
createSelector() { | ||
const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions) | ||
this.selector = makeSelectorStateful(sourceSelector, this.store) | ||
this.selector.run(this.props) | ||
return makeSelectorStateful(sourceSelector, this.store) | ||
} | ||
|
||
initSubscription() { | ||
|
@@ -209,9 +214,9 @@ export default function connectAdvanced( | |
} | ||
|
||
onStateChange() { | ||
this.selector.run(this.props) | ||
this.state.selector.run(this.props) | ||
|
||
if (!this.selector.shouldComponentUpdate) { | ||
if (!this.state.selector.shouldComponentUpdate) { | ||
this.notifyNestedSubs() | ||
} else { | ||
this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate | ||
|
@@ -247,7 +252,8 @@ export default function connectAdvanced( | |
} | ||
|
||
render() { | ||
const selector = this.selector | ||
const selector = this.state.selector | ||
|
||
selector.shouldComponentUpdate = false | ||
|
||
if (selector.error) { | ||
|
@@ -263,13 +269,13 @@ export default function connectAdvanced( | |
Connect.childContextTypes = childContextTypes | ||
Connect.contextTypes = contextTypes | ||
Connect.propTypes = contextTypes | ||
Connect.getDerivedStateFromProps = getDerivedStateFromProps | ||
|
||
if (process.env.NODE_ENV !== 'production') { | ||
Connect.prototype.componentWillUpdate = function componentWillUpdate() { | ||
Connect.prototype.componentDidUpdate = function componentDidUpdate() { | ||
// We are hot reloading! | ||
if (this.version !== version) { | ||
this.version = version | ||
this.initSelector() | ||
|
||
// If any connected descendants don't hot reload (and resubscribe in the process), their | ||
// listeners will be lost when we unsubscribe. Unfortunately, by copying over all | ||
|
@@ -287,10 +293,16 @@ export default function connectAdvanced( | |
this.subscription.trySubscribe() | ||
oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) | ||
} | ||
|
||
const selector = this.createSelector() | ||
selector.run(this.props) | ||
this.setState({selector}) | ||
} | ||
} | ||
} | ||
|
||
polyfill(Connect) | ||
|
||
return hoistStatics(Connect, WrappedComponent) | ||
} | ||
} |
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.
Why do you need the polyfill when you changed all the deprecated functions?
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.
He's using
getDerivedStateFromProps
.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 am using that too without a polyfill. No more warnings (in my own code).
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.
It's for use with older versions of React. It's not to deal with deprecation warnings, it's to polyfill those lifecycle methods for React <= 16.2.
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.
Ah right, didn't think it that way around.
That brings me to a question for my own lib. What if I want to support the new and old Context API I guess I could just check if
React.createContext
is a function and then use it if avail?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.
@pke : there's a distinct difference in how you declare and use the old and new context APIs. See my PR in #898 for an example.
We may need to do that check for
React.createContext
so we can use a polyfill likecreate-react-context
, but the old and new context APIs aren't interchangeable.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.
New context API is probably going to require breaking changes to React Redux's public API for supporting multiple stores, making it a major version bump.