Skip to content

Conversation

youknowriad
Copy link
Contributor

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

  • Unit/E2e tests pass
  • Gutenberg works

@youknowriad youknowriad added the [Package] Data /packages/data label Jun 7, 2018
@youknowriad youknowriad self-assigned this Jun 7, 2018
@youknowriad youknowriad requested a review from aduth June 7, 2018 10:54
@youknowriad youknowriad changed the title Data Module: Refactor Higher-order components to avoid the use of com… Data Module: Refactor Higher-order components to avoid the use of componentWillMount Jun 7, 2018
@gziolo
Copy link
Member

gziolo commented Jun 7, 2018

It looks good as far as I can see, but it definitely should be confirmed by @aduth 👍

@@ -280,44 +279,46 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W
* @type {boolean}
*/
this.shouldComponentUpdate = false;
this.state = {
mergeProps: mapStateToProps( select, props ) || {},
Copy link
Member

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 ) || {};
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screen shot 2018-06-08 at 12 57 25

Shouldn't it be?

const defaultMergeProps = {};
static function getNextMergeProps( props ) {
	return mapStateToProps( select, props ) || dedefaultMergeProps;
}

Unless it doesn't matter, I'm not sure :)

Copy link
Member

@aduth aduth Jun 8, 2018

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

Copy link
Member

@aduth aduth Jun 8, 2018

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:

https://github.com/WordPress/packages/blob/c24da3afe3fa4b1ccd234f8c2ec766d43201c1ce/packages/is-shallow-equal/objects.js#L16-L18

This would only happen on cases where we're returning undefined (falsey), which for withSelect is probably not so frequent.

componentWillReceiveProps( nextProps ) {
if ( ! isShallowEqual( nextProps, this.props ) ) {
this.runSelection( nextProps );
this.shouldComponentUpdate = true;
}
}

componentDidMount() {
this.canRunSelection = true;
Copy link
Member

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?

Copy link
Contributor Author

@youknowriad youknowriad Jun 7, 2018

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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runSelection calls setState?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

@youknowriad youknowriad force-pushed the update/data-module-deprecated-react-lifecycle branch from 635f54f to ffc8e27 Compare June 11, 2018 10:04
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@youknowriad youknowriad merged commit 95ef796 into master Jun 19, 2018
@youknowriad youknowriad deleted the update/data-module-deprecated-react-lifecycle branch June 19, 2018 11:20
@youknowriad youknowriad added this to the 3.1 milestone Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants