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

Merged
merged 4 commits into from Jun 19, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Jun 7, 2018

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 self-assigned this Jun 7, 2018

@youknowriad youknowriad requested a review from aduth Jun 7, 2018

@youknowriad youknowriad changed the title from Data Module: Refactor Higher-order components to avoid the use of com… to Data Module: Refactor Higher-order components to avoid the use of componentWillMount Jun 7, 2018

@gziolo

This comment has been minimized.

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

This comment has been minimized.

@aduth

aduth Jun 7, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Jun 8, 2018

Member

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 :)

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

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

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

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;

This comment has been minimized.

@aduth

aduth Jun 7, 2018

Member

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?

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

you can't setState if the component is not mounted so selection can be only run when the component is really mounted.

This comment has been minimized.

@aduth

aduth Jun 7, 2018

Member

How do we get from setState to having runSelection being called?

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

runSelection calls setState?

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

You mentioned that the issue is setState could be called between constructor and componentDidMount. When will that occur?

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

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.

This comment has been minimized.

@aduth

aduth Jun 8, 2018

Member

We should track it down and get a unit test in place which would fail without this assignment.

This comment has been minimized.

@youknowriad

youknowriad Jun 11, 2018

Contributor

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

@gziolo

gziolo approved these changes Jun 19, 2018

LGTM 👍

@youknowriad youknowriad merged commit 95ef796 into master Jun 19, 2018

2 checks passed

codecov/project 46.69% (+<.01%) compared to ebb289f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/data-module-deprecated-react-lifecycle branch Jun 19, 2018

@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