Remove Component Stack from React Logged Warnings and Error Reporting #30308
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
commented
Jul 10, 2024
| args = args.concat([stack]); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the effective change.
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
500e177
to
b61ec63
Compare
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
b61ec63
to
6acb192
Compare
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
6acb192
to
e29b369
Compare
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
e29b369
to
f82c48d
Compare
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
f82c48d
to
6cd51c7
Compare
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
9d1bb1d
to
9a22b95
Compare
This doesn't test for general warnings but for actual error logging so they use a different mechanism without the polyfill. We can't really test React.captureOwnerStack() here since this is tracking default behavior. Could maybe test reading it inside the mock?
Since third parties would be covered by a polyfill/devtools.
They now sometimes include a normalized component stack if one was logged.
sebmarkbage
force-pushed
the
rmcomponentstack
branch
from
9a22b95
to
92b1e9d
Compare
hoxyq
approved these changes
Jul 12, 2024
sebmarkbage
added a commit
that referenced
this pull request
Jul 12, 2024
Stacked on #30308. This is now a noop module so we can stop applying the transform of console.error using the Babel plugin in the mainline builds. I'm keeping the transform for RN/WWW for now although it might be nice if the transform moved into those systems as it gets synced instead of keeping it upstream. In jest tests we're already not running the forks for RN/WWW so we don't need it at all there.
github-actions bot
pushed a commit
that referenced
this pull request
Jul 12, 2024
Stacked on #30308. This is now a noop module so we can stop applying the transform of console.error using the Babel plugin in the mainline builds. I'm keeping the transform for RN/WWW for now although it might be nice if the transform moved into those systems as it gets synced instead of keeping it upstream. In jest tests we're already not running the forks for RN/WWW so we don't need it at all there. DiffTrain build for commit ff89ba7.
github-actions bot
pushed a commit
that referenced
this pull request
Jul 12, 2024
Stacked on #30308. This is now a noop module so we can stop applying the transform of console.error using the Babel plugin in the mainline builds. I'm keeping the transform for RN/WWW for now although it might be nice if the transform moved into those systems as it gets synced instead of keeping it upstream. In jest tests we're already not running the forks for RN/WWW so we don't need it at all there. DiffTrain build for [ff89ba7](ff89ba7)
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30308. This is now a noop module so we can stop applying the transform of console.error using the Babel plugin in the mainline builds. I'm keeping the transform for RN/WWW for now although it might be nice if the transform moved into those systems as it gets synced instead of keeping it upstream. In jest tests we're already not running the forks for RN/WWW so we don't need it at all there.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
…facebook#30308) React transpiles some of its own `console.error` calls into a helper that appends component stacks to those calls. However, this doesn't cover user space `console.error` calls - which includes React helpers that React has moved into third parties like createClass and prop-types. The idea is that any user space component can add a warning just like React can which is why React DevTools adds them too if they don't already exist. Having them appended in both places is tricky because now you have to know whether to remove them from React's logs. Similarly it's often common for server-side frameworks to forget to cover the `console.error` logs from other sources since React DevTools isn't active there. However, it's also annoying to get component stacks clogging the terminal - depending on where the log came from. In the future `console.createTask()` will cover this use case natively and when available we don't append them at all. The new strategy relies on either: - React DevTools existing to add them to React logs as well as third parties. - `console.createTask` being supported and surfaced. - A third party framework showing the component stack either in an Error Dialog or appended to terminal output. For a third party to be able to implement this they need to be able to get the component stack. To get the component stack from within a `console.error` call you need to use the `React.captureOwnerStack()` helper which is only available in `enableOwnerStacks` flag. However, it's possible to polyfill with parent stacks using internals as a stop gap. There's a question of whether React 19 should just go out with `enableOwnerStacks` to expose this but regardless I think it's best it doesn't include component stacks from the runtime for consistency. In practice it's not really a regression though because typically either of the other options exists and error dialogs don't implement `console.error` overrides anyway yet. SSR terminals might miss them but they'd only have them in DEV warnings to begin with an a subset of React warnings. Typically those are either going to happen on the client anyway or replayed. Our tests are written to assert that component stacks work in various scenarios all over the place. To ensure that this keeps working I implement a "polyfill" that is similar to that expected a server framework might do - in `assertConsoleErrorDev` and `toErrorDev`. This PR doesn't yet change www or RN since they have their own forks of consoleWithStackDev for now.
This was referenced Jul 25, 2024
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.
React transpiles some of its own
console.errorcalls into a helper that appends component stacks to those calls. However, this doesn't cover user spaceconsole.errorcalls - which includes React helpers that React has moved into third parties like createClass and prop-types.The idea is that any user space component can add a warning just like React can which is why React DevTools adds them too if they don't already exist. Having them appended in both places is tricky because now you have to know whether to remove them from React's logs.
Similarly it's often common for server-side frameworks to forget to cover the
console.errorlogs from other sources since React DevTools isn't active there. However, it's also annoying to get component stacks clogging the terminal - depending on where the log came from.In the future
console.createTask()will cover this use case natively and when available we don't append them at all.The new strategy relies on either:
console.createTaskbeing supported and surfaced.For a third party to be able to implement this they need to be able to get the component stack. To get the component stack from within a
console.errorcall you need to use theReact.captureOwnerStack()helper which is only available inenableOwnerStacksflag. However, it's possible to polyfill with parent stacks using internals as a stop gap. There's a question of whether React 19 should just go out withenableOwnerStacksto expose this but regardless I think it's best it doesn't include component stacks from the runtime for consistency.In practice it's not really a regression though because typically either of the other options exists and error dialogs don't implement
console.erroroverrides anyway yet. SSR terminals might miss them but they'd only have them in DEV warnings to begin with an a subset of React warnings. Typically those are either going to happen on the client anyway or replayed.Our tests are written to assert that component stacks work in various scenarios all over the place. To ensure that this keeps working I implement a "polyfill" that is similar to that expected a server framework might do - in
assertConsoleErrorDevandtoErrorDev.This PR doesn't yet change www or RN since they have their own forks of consoleWithStackDev for now.