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 the componentWillReceiveProps lifecycle usage #7395

Merged
merged 7 commits into from Jun 21, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Jun 20, 2018

use componentDidUpdate/getDerivedPropsFromState instead

There's one remaining usage of componentWillReceiveProps in the TinyMCE component but I'm not certain yet how to replace it since this component has componentShouldUpdate returning false.

Also componentWillUpdate in fills is not updated because I think it's important to keep the right order of fills. I'll defer to someone else here cc @aduth

Testing instructions

It should be fine but there are a lot of things that behave a little bit differently:

  • Time picker
  • Dropdown onToggle callback
  • Tags input
  • Unselecting captions in galleries and images
  • Document title updates
  • Preview button
  • Post publish panel

@youknowriad youknowriad self-assigned this Jun 20, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Jun 20, 2018

@@ -297,9 +303,13 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W
return this.shouldComponentUpdate;
}
componentWillReceiveProps( nextProps ) {

This comment has been minimized.

@youknowriad

youknowriad Jun 20, 2018

Contributor

This changes could be impactful because of the order between the lifecycle calls, but in my testing, it's working as expected.

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

This seems like it will force a second render for every connected component on store updates.

* Boolean tracking when can we run selection
* The selection can be run only when the component is mounted.
*/
this.shouldRunSelection = false;

This comment has been minimized.

@youknowriad

youknowriad Jun 20, 2018

Contributor

I know I removed this previously @aduth but it looks like it's necessary, hard to reproduce in unit tests but without it, you notice some warnings about setState being called on components not mounted.

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

We should just move the subscribe into componentDidMount.

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

That's not possible because the subscribe of parent components should happen before children.

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

That's not possible because the subscribe of parent components should happen before children.

Why? Do we test or document this anywhere?

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

Yes, I have a comment on the constructor. The comment could be made clearer.

For example:

if you have a BlockList component showing a Block component, you want the subscribe to happen first in BlockList to unmount the Block if it gets deleted before the mapStateToProps runs in the child

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

if you have a BlockList component showing a Block component, you want the subscribe to happen first in BlockList to unmount the Block if it gets deleted before the mapStateToProps runs in the child

Seems reasonable. I'll see if I can put together a unit test for this as part of #7431.

@@ -93,11 +93,11 @@ class FormatToolbar extends Component {
}
}
componentWillReceiveProps( nextProps ) {
if ( this.props.selectedNodeId !== nextProps.selectedNodeId ) {
componentDidUpdate( prevProps ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 20, 2018

Member

In this case we are just setting the state so getDerivedStateFromProps sounds like a good git. The downside is that we will need to keep a reference to selectedNodeId in state but I think it is worth it to avoid double render calls.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 20, 2018

Member

According to the React rfcs mirroring the values in state sounds like the way to go: https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md#state-derived-from-propsstate

This comment has been minimized.

@youknowriad

youknowriad Jun 20, 2018

Contributor

But we can't because we use the previous props and since getDerivedStateFromProps is static we can't access those.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 20, 2018

Member

We can save the previous selectedNodeId in the state. And then in getDerivedStateFromProps we compare the new prop with the previous (saved on the state), the same they suggest in https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md#state-derived-from-propsstate.

@@ -32,11 +32,11 @@ class PostTextEditor extends Component {
};
}
componentWillReceiveProps( nextProps ) {
componentDidUpdate( prevProps ) {

This comment has been minimized.

@youknowriad youknowriad changed the title from Remove the componentWillReceiveProps lifecycle usage, to Remove the componentWillReceiveProps lifecycle usage Jun 20, 2018

@gziolo

gziolo approved these changes Jun 21, 2018

In general looks great, lots of code to update. I left 3 comments but I see they might be not actionable seeing this:

There's one remaining usage of componentWillReceiveProps in the TinyMCE component but I'm not certain yet how to replace it since this component has componentShouldUpdate returning false.

this.editorNode.setAttribute( IS_PLACEHOLDER_VISIBLE_ATTR_NAME, isPlaceholderVisibleString );
}
}
componentWillReceiveProps( nextProps ) {

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

componentWillReceiveProps is still here, but I see configureIsPlaceholderVisible moved. Is it intentional? Should we use componentDidUpdate in here?

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

Commented about this one on the PR description. No clear alternative here (the Fill one as well).

if ( ! isEqual( nextProps, this.props ) ) {
this.fetch( nextProps );
componentDidUpdate( prevProps ) {
if ( ! isEqual( prevProps, this.props ) ) {

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Unrelated, should we use isShallowEqual like in other places when comparing props?

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

Probably, I'm not familiar with this component to tell though. Also, it looks like it's not being used in Gutenberg at all.

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

Also, it looks like it's not being used in Gutenberg at all.

Can you elaborate on this point?

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

The ServerSideRender component is not used in Gutenberg.

*/
function computeDerivedState( props ) {
return {
persistedValue: props.value,

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

This one is tricky, but I see now why it exists.

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

This one is tricky, but I see now why it exists.

It's unclear to me. Should we have left an inline comment?

@gziolo gziolo added this to the 3.1 milestone Jun 21, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

It tests well, I didn't find any regressions. 🚢

@youknowriad youknowriad merged commit 86011f1 into master Jun 21, 2018

2 checks passed

codecov/project 46.9% (-0.02%) compared to 82cfb4f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/remove-component-will-receive-props branch Jun 21, 2018

@@ -367,8 +377,8 @@ export const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent
this.setProxyProps( props );
}
componentWillUpdate( nextProps ) {
this.setProxyProps( nextProps );
componentDidUpdate() {

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

I don't think this conversion is correct: componentWillUpdate runs before render, and setProxyProps is only assigning an instance variable, so on its own it won't incur another render. Therefore, by changing to componentDidUpdate we risk the new proxy props not being applied correctly (or at least until the next render).

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

Yes, I was aware of this difference but in my testing it's working fine. What alternative do you propose?

@@ -571,6 +571,7 @@ describe( 'withSelect', () => {
expect( wrapper.childAt( 0 ).props() ).toEqual( { foo: 'OK', propName: 'foo' } );
wrapper.setProps( { propName: 'bar' } );
wrapper.update();

This comment has been minimized.

@aduth

aduth Jun 21, 2018

Member

Why should we have to force a re-render? This seems quite problematic.

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

I think it's due to how enzyme works, we do this in several tests. In theory it should rerender automatically when we call setProps but it doesn't.

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Yes, Enzyme specific thing introduced with 16.x line, it usually isn't needed, but sometimes it's the only way to make tests work :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment