Skip to content
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

Add warning for componentDidReceiveProps() #11479

Merged
merged 2 commits into from Nov 7, 2017
Merged

Conversation

iamtommcc
Copy link
Contributor

As per Dan's suggestion on Twitter, this is a tiny little PR that adds componentDidReceiveProps to the list of invalid lifecycle methods to throw warnings for.

acdlite
acdlite previously requested changes Nov 7, 2017
noComponentDidReceiveProps,
'%s has a method called ' +
'componentDidReceiveProps(). But there is no such lifecycle method. ' +
'Did you mean componentDidUpdate()?',
Copy link
Collaborator

@acdlite acdlite Nov 7, 2017

Choose a reason for hiding this comment

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

Could also have meant componentWillReceiveProps()? Let's include both

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in most cases people would mean componentWillReceiveProps.

Maybe we should say

If you meant to update the state in response to changing props, use componentWillReceiveProps(). If you meant to perform manual DOM manipulations or fetch data, use componentDidUpdate().

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that, though will also be seen for non-DOM users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

If you meant to update the state in response to changing props, use componentWillReceiveProps(). If you meant to fetch data or run some code after React has updated the UI, use componentDidUpdate().

Copy link
Collaborator

Choose a reason for hiding this comment

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

"some code" -> "side-effects or mutations"

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@gaearon gaearon merged commit 48012ef into facebook:master Nov 7, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 7, 2017

LGTM. Thanks!

@trueadm
Copy link
Contributor

trueadm commented Nov 7, 2017

Isn't this a major change? At a previous company I worked at, we added a componentDidReceiveProps method to a HOC that didn't fire it if setState() was used to update itself. I believe this would now break that user-land method?

A better idea might be to reserve method names (DEV only) like these in a future version. This will lock them down so no-one can use them and in turn we can provide better errors in general?

@gaearon
Copy link
Collaborator

gaearon commented Nov 7, 2017

Not break, but it will add a warning. We routinely add warnings in minor versions and if that breaks your build, we recommend locking down the dependencies.

In this case my feeling is that it's more likely this will be beneficial (and possibly uncover unintentional mistakes) to most users. People who overloaded it with a special meaning can always rename their version with a global find-replace.

I'd say in general it is a bad idea to name non-React things with names like componentDid*. Since people reading that code will certainly think it's part of React, won't be able to find it in the docs, and potentially can even use it outside of the context where it works (e.g. in that HOC).

@swyxio
Copy link
Contributor

swyxio commented Nov 7, 2017

I'd say in general it is a bad idea to name non-React things with names like componentDid*

naming things is hard, might want to formalize this somewhere in the docs? any other naming issues that people usually run into? i'll move this over to the docs repo but just getting ideas

@gaearon
Copy link
Collaborator

gaearon commented Nov 7, 2017

To be honest I don't think this is common enough to highlight it in docs. If anything, it's better not to give people the idea to create such methods by mentioning it :-)

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* Add warning for componentDidReceiveProps()

* Adjust message for componentDidReceiveProps() warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants