tweak unsafe lifecycle warnings#15431
Closed
threepointone wants to merge 11 commits intofacebook:masterfrom
Closed
tweak unsafe lifecycle warnings#15431threepointone wants to merge 11 commits intofacebook:masterfrom
threepointone wants to merge 11 commits intofacebook:masterfrom
Conversation
added 8 commits
April 3, 2019 17:11
|
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 4221565...1ba1642 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
Contributor
|
Possible alternate wording. Don't know if it's great either, just sharing for discussion.
|
Contributor
Author
|
Updated with suggestions, did the same for the server rendered warning which I'd missed, updated tests. I also updated the screenshot in the description. ^ |
Contributor
Author
|
a followup 'todo' - make a warnings page we can point folks to, that's basically the blogpost, but reordered to emphasize solutions/suggestions. |
Contributor
Author
|
I forgot to add |
bvaughn
reviewed
Apr 18, 2019
…ll be logged in strict mode".
Contributor
Author
|
updated PR and screenshot |
bvaughn
approved these changes
Apr 18, 2019
| '- If you initialize state in componentWillMount, move this logic into the constructor.\n' + | ||
| '- If you fetch data or perform other side effects in componentWillMount, ' + | ||
| 'move this logic into componentDidMount.\n' + | ||
| '- To rename all deprecated lifecycles to their new names, you can run ' + |
Contributor
There was a problem hiding this comment.
Maybe we should give a hint as to why folks might not want to just rename and ignore? Maybe we could replace
"To rename all deprecated lifecycles to their new names, ..."
...with
"To prevent this warning from being logged to the console, ..."
...so it sounds a little more like you're just choosing to ignore the warning without fixing the problem?
(here and below)
Contributor
Author
There was a problem hiding this comment.
How about "To ignore this warning from being logged to the console..."
Contributor
Author
There was a problem hiding this comment.
rather, "to disable this warning..."
Contributor
Author
There was a problem hiding this comment.
It's so wordy :(
Contributor
Author
There was a problem hiding this comment.
I think that we're saying the warning will still log in strict mode, and that we've moved this to the bottom of the list, is sufficient. I'll fight you on this briannson.
| 'We suggest doing one of the following:\n' + | ||
| '- If you fetch data or perform other side effects in componentWillUpdate, ' + | ||
| 'move this logic into componentDidUpdate.\n' + | ||
| "- If you're reading DOM properties before an update, " + |
Contributor
There was a problem hiding this comment.
Legit use cases for getSnapshotBeforeUpdate seem so uncommon, I wonder if it's worth mentioning here at all? I don't have a strong preference one way or another, but a slight preference to maybe not mention it here for fear of encouraging bad patterns for those maybe lacking context of when the method should be used.
Contributor
Author
There was a problem hiding this comment.
I think I'll leave it here. We seem fairly comprehensive for the others, it would be frustrating if the dev had to go to a doc to discover a new api for just this one case.
Contributor
There was a problem hiding this comment.
I guess I'm concerned that if people just move stuff to that method, without the added context of why it exists, they might be tempted to do mutations/side effects in it.
Contributor
Author
There was a problem hiding this comment.
They would probably try componentDidUpdate before that though? I dunno tbh. I could remove this, but seems like we should use same reasoning to remove getDerivedStateFromProps if so.
ziishaned
approved these changes
Apr 18, 2019
Contributor
Author
|
updated PR and screenshot |
Collaborator
|
I would not try to explain when to use which lifecycle method in the warning text. I think it is just too small a space and we should encourage people to read the blog posts instead. It's better to make the warning easier to grok quickly and then have a place for people to read when they actually want to get into it. (Most people will need to read more to be sure in their changes anyway.) My suggestion would be to write something along the following lines: In non-strict mode: In strict mode if componentWillMount is used: In strict mode if componentWillMount is not used: I think it's also important there is a place to learn more about the auto-rename script. People are gonna be a little hesitant unless they know what it'll do. (Which files will it run on? etc) |
Contributor
Author
|
Sorry I got a while to get back to this. Some thoughts -
Trying to reconcile this with the suggestion, it's only one sentence shorter than what the PR currently has. I think a quick reference of alternatives is indeed valuable, and don't want folks to have to read a long post for every warning (and the link is pretty obvious, so they will go there if they don't comprehend immediately what to do. I agree we should have different warning with strict/non strict mode, hacking on it asap. |
|
This messaging doesn't harmonize well with the current state of the server renderer, which works with |
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 10, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 10, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 10, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
Contributor
Author
|
abandoning this for #16103, which uses all the feedback from here, and then some. see y'all there! |
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 11, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 12, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 12, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 12, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 12, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 15, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 15, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jul 15, 2019
- redoes facebook#15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
threepointone
pushed a commit
that referenced
this pull request
Jul 15, 2019
- redoes #15431 from scratch, taking on the feedback there - unifies the messaging between "deprecated" and UNSAFE_ lifecycle messages. It reorganizes ReactStrictModeWarnings.js to capture and flush all the lifecycle warnings in one procedure each. - matches the warning from ReactPartialRenderer to match the above change - passes all the tests - this also turns on `warnAboutDeprecatedLifecycles` for the test renderer. I think we missed doing so it previously. In a future PR, I'll remove the feature flag altogether. - this DOES NOT do the same treatment for context warnings, I'll do that in another PR too
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tbh it's still not great, but started a PR to bikeshed. lemme know how to make this better.
This is what it looks like - (updated 18/05)
