-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Data Module: Refactor Higher-order components to avoid the use of componentWillMount #7206
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
Data Module: Refactor Higher-order components to avoid the use of componentWillMount #7206
Conversation
It looks good as far as I can see, but it definitely should be confirmed by @aduth 👍 |
packages/data/src/index.js
Outdated
@@ -280,44 +279,46 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W | |||
* @type {boolean} | |||
*/ | |||
this.shouldComponentUpdate = false; | |||
this.state = { | |||
mergeProps: mapStateToProps( select, props ) || {}, |
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.
Minor: If we rely on the behavior to fall back to object, wonder if we should put this in a common getter function between the constructor
and runSelection
usage, e.g.
static function getNextMergeProps( props ) {
return mapStateToProps( select, props ) || {};
}
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.
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.
Unless it doesn't matter, I'm not sure :)
It shouldn't matter. We're running a shallow equality on the result, so, with your example:
isShallowEqual( test(), test() );
// true
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.
That said, it could be a micro-optimization since strict equality is a shortcutting case in @wordpress/is-shallow-equal
:
This would only happen on cases where we're returning undefined
(falsey), which for withSelect
is probably not so frequent.
packages/data/src/index.js
Outdated
componentWillReceiveProps( nextProps ) { | ||
if ( ! isShallowEqual( nextProps, this.props ) ) { | ||
this.runSelection( nextProps ); | ||
this.shouldComponentUpdate = true; | ||
} | ||
} | ||
|
||
componentDidMount() { | ||
this.canRunSelection = true; |
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.
Can you clarify what the potential issue is that warranted the assignment here? Can we add a unit test which would otherwise fail without it?
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.
you can't setState
if the component is not mounted so selection can be only run when the component is really mounted.
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.
How do we get from setState
to having runSelection
being called?
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.
runSelection
calls setState
?
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.
You mentioned that the issue is setState
could be called between constructor
and componentDidMount
. When will that occur?
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.
Oh, I don't know exactly :) I just had errors when loading Gutenberg when I tried.
Also, I guess in an async React mode, Components can be initialized but not mounted directly which means a lot of subscribe
calls will happen in between.
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 should track it down and get a unit test in place which would fail without this assignment.
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.
Haha, it looks like I'm not able to reproduce anymore :). Maybe it was related to another fix I did. I'm removing this for now
635f54f
to
ffc8e27
Compare
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.
LGTM 👍
In #7202 I'm trying to enable StrictMode and this surfaces a big number of files we're relying on deprecated Lifecycle methods.
This PR updates the data module HoC to avoid using
componentWillMount
lifecycle method.Testing instructions