-
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 component stacks to (almost) all warnings #17586
Conversation
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 1569cad:
|
16b5120
to
1569cad
Compare
Details of bundled changes.Comparing: 12c0004...1569cad react
react-dom
react-noop-renderer
react-reconciler
react-art
react-test-renderer
react-cache
react-native-renderer
create-subscription
ReactDOM: size: 0.0%, gzip: 0.0% React: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: 12c0004...1569cad react
react-dom
react-test-renderer
react-native-renderer
react-interactions
react-noop-renderer
react-art
react-cache
react-reconciler
create-subscription
ReactDOM: size: 0.0%, gzip: 🔺+0.1% React: size: 0.0%, gzip: 🔺+0.1% Size changes (experimental) |
There's a very minor bundle size for non-React-using entry points increase due to more code paths pulling in the glue code that's only used in DEV. I'll fix that in a follow up that does this at the build time level. |
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.
More component stacks is good 👍
As the title says. This adds component stacks to all warnings except the ones where a package doesn't have peer or dep on React (such as
react-is
). Concretely, we switch over fromwarningWithoutStack
towarning
, and fromlowPriorityWarningWithoutStack
tolowPriorityWarning
, wherever possible. Tests are adjusted accordingly.There are three reasons for this.
console.error
andconsole.warn
in the source code, and having a Babel plugin call the wrapper with extra logic. So it's inconvenient to have this distinction.What if the stack is misleading or not concrete enough? For example, maybe we have a better stack we want to pass. We do the same thing DevTools does in that case. If the last argument already looks like a component stack, we don't append an extra one.
Notably, this adds component stacks to SSR hydration mismatch warnings. They can still sometimes be misleading but it's better than nothing.
Some tests still have
{withoutStack: true}
. This is not because we can't emit them, but because in those particular tests we're just not inside of a tree. So we're keeping the assertion mechanism for that to avoid regressions.In a follow-up, I will codemod to
console.error
/console.warn
calls and a Babel plugin for the wrappers. This will also allow us to choose the right version of the wrapper depending on whether a package depends on React or not.