Update legacy context warning message #17882
Closed
Conversation
This file contains 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
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 32cdbda:
|
Details of bundled changes.Comparing: 0c04aca...32cdbda react-test-renderer
react-native-renderer
react-art
react-dom
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: 0c04aca...32cdbda react-native-renderer
react-reconciler
react-dom
react-art
react-test-renderer
Size changes (stable) |
Fix flow
trueadm
force-pushed
the
legacy-context-warning
branch
from
294effd
to
8baff7e
Compare
gaearon
reviewed
Jan 21, 2020
bvaughn
reviewed
Jan 21, 2020
packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js
Show resolved
Hide resolved
gaearon
reviewed
Jan 22, 2020
packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js
Outdated
Show resolved
Hide resolved
gaearon
reviewed
Jan 22, 2020
gaearon
requested changes
Jan 22, 2020
There was a problem hiding this comment.
I understand your original concern better now. I agree with @bvaughn we shouldn't regress DEV perf here since it's a lot of Fibers. I think replacing didWarnAboutLegacyContext with a Map of Fiber Type => Number of Occurrences makes sense so that we don't have to ask every component's stack.
I think you'll still need a Set to track which types we've flushed the warnings for. So that we don't consider their stacks in next messages.
Does this make sense?
trueadm
force-pushed
the
legacy-context-warning
branch
from
ce97e99
to
ccbd9d9
Compare
Address more feedback Address more feedback
trueadm
force-pushed
the
legacy-context-warning
branch
from
ccbd9d9
to
12d95b6
Compare
bvaughn
reviewed
Jan 23, 2020
|
@gaearon I've made those changes, let me know if you're happy with them. |
bvaughn
reviewed
Jan 24, 2020
bvaughn
reviewed
Jan 24, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
After speaking to @gaearon, we spoke about how we can improve the warning message when using StrictMode with legacy context. Previously, we would output all the offending components and the component stack of the StrictMode, which wasn't that useful in practice. With the changes in this PR, we now output the component stack of the component that most frequently had legacy context warnings in StrictMoode.