-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Conversation
noComponentDidReceiveProps, | ||
'%s has a method called ' + | ||
'componentDidReceiveProps(). But there is no such lifecycle method. ' + | ||
'Did you mean componentDidUpdate()?', |
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.
Could also have meant componentWillReceiveProps()
? Let's include both
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.
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, usecomponentDidUpdate()
.
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.
I like that, though will also be seen for non-DOM users.
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.
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, usecomponentDidUpdate()
.
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.
"some code" -> "side-effects or mutations"
?
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. Thanks! |
Isn't this a major change? At a previous company I worked at, we added a 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? |
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 |
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 |
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 :-) |
* Add warning for componentDidReceiveProps() * Adjust message for componentDidReceiveProps() warning
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.