-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Set the current fiber to the source of the error during error reporting #29044
Conversation
22fdb4b
to
9e54e8f
Compare
[root] | ||
<ErrorBoundary> ✕ | ||
<ErrorBoundary> |
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.
What happens here is that now the console.error happens in the context of the source of the error, not in the error boundary, but that node gets deleted so it's no longer part of the tree.
That's what happens to the console.error already which is why it was only one marked here and not two (one for console.error and one for throw). (It was one before this PR and two before the stack but the other one is representing the console.error in TestRenderer, so it was the wrong one.)
It seems like maybe this UI should be adjusted but we have similar problems with infinitely suspended components whose fiber never commits.
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.
It seems like maybe this UI should be adjusted but we have similar problems with infinitely suspended components whose fiber never commits.
Ideally you'd still be able to narrow down the console.error. It was likely confusing to attach the error to the error boundary when that's not where it originated. But some indicator that the error originated from somewhere within this tree would be nice.
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 don't think it was attached to the error boundary but dropped before too. The thrown was attached to the error boundary.
The other previous error came from the TestRenderer warning about using test renderer.
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.
Logs are tricky because there can be a done of completely unrelated logs in a subtree like the whole page that unmount. Not necessarily associated with the error that triggered the error boundary.
However, the actual error not being marked is clearly an issue.
9e54e8f
to
bd02706
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
bd02706
to
1ea6ffc
Compare
52fe9e2
to
66fb6fb
Compare
This lets us expose the component stack to the error reporting that happens here as console.error patching, but more importantly for "owner stacks" this will be able to set the native async stacks to the original fiber. We now use the normal console.error management to add a component stack to the end. # Conflicts: # packages/shared/consoleWithStackDev.js
66fb6fb
to
0f2b634
Compare
…ng (#29044) This lets us expose the component stack to the error reporting that happens here as `console.error` patching. Now if you just call `console.error` in the error handlers it'll get the component stack added to the end by React DevTools. However, unfortunately this happens a little too late so the Fiber will be disconnected with its `.return` pointer set to null already. So it'll be too late to extract a parent component stack from but you can at least get the stack from source to error boundary. To work around this I manually add the parent component stack in our default handlers when owner stacks are off. We could potentially fix this but you can also just include it yourself if you're calling `console.error` and it's not a problem for owner stacks. This is not a problem for owner stacks because we'll still have those and so for those just calling `console.error` just works. However, the main feature is that by letting React add them, we can switch to using native error stacks when available. DiffTrain build for commit 2e540e2.
…ng (#29044) This lets us expose the component stack to the error reporting that happens here as `console.error` patching. Now if you just call `console.error` in the error handlers it'll get the component stack added to the end by React DevTools. However, unfortunately this happens a little too late so the Fiber will be disconnected with its `.return` pointer set to null already. So it'll be too late to extract a parent component stack from but you can at least get the stack from source to error boundary. To work around this I manually add the parent component stack in our default handlers when owner stacks are off. We could potentially fix this but you can also just include it yourself if you're calling `console.error` and it's not a problem for owner stacks. This is not a problem for owner stacks because we'll still have those and so for those just calling `console.error` just works. However, the main feature is that by letting React add them, we can switch to using native error stacks when available. DiffTrain build for [2e540e2](2e540e2)
) Stacked on #29044. To work with `console.createTask(...).run(...)` we need to be able to run a function in the scope of the task. The main concern with this, other than general performance, is that it might add more stack frames on very deep stacks that hit the stack limit. Such as with the commit phase where we recursively go down the tree. These callbacks aren't really necessary in the recursive part but only in the shallow invocation of the commit phase for each tag. So we could refactor the commit phase so that only the shallow part at each level is covered this way.
) Stacked on #29044. To work with `console.createTask(...).run(...)` we need to be able to run a function in the scope of the task. The main concern with this, other than general performance, is that it might add more stack frames on very deep stacks that hit the stack limit. Such as with the commit phase where we recursively go down the tree. These callbacks aren't really necessary in the recursive part but only in the shallow invocation of the commit phase for each tag. So we could refactor the commit phase so that only the shallow part at each level is covered this way. DiffTrain build for commit b078c81.
Full list of changes: * chore[react-devtools]: improve console arguments formatting before passing it to original console ([hoxyq](https://github.com/hoxyq) in [#29873](#29873)) * chore[react-devtools]: unify console patching and default to ansi escape symbols ([hoxyq](https://github.com/hoxyq) in [#29869](#29869)) * chore[react-devtools/backend]: remove consoleManagedByDevToolsDuringStrictMode ([hoxyq](https://github.com/hoxyq) in [#29856](#29856)) * chore[react-devtools/extensions]: make source maps url relative ([hoxyq](https://github.com/hoxyq) in [#29886](#29886)) * fix[react-devtools] divided inspecting elements between inspecting do… ([vzaidman](https://github.com/vzaidman) in [#29885](#29885)) * [Fiber] Create virtual Fiber when an error occurs during reconcilation ([sebmarkbage](https://github.com/sebmarkbage) in [#29804](#29804)) * fix[react-devtools] component badge in light mode is now not invisible ([vzaidman](https://github.com/vzaidman) in [#29852](#29852)) * Remove Warning: prefix and toString on console Arguments ([sebmarkbage](https://github.com/sebmarkbage) in [#29839](#29839)) * Add jest lint rules ([rickhanlonii](https://github.com/rickhanlonii) in [#29760](#29760)) * [Fiber] Track the Real Fiber for Key Warnings ([sebmarkbage](https://github.com/sebmarkbage) in [#29791](#29791)) * fix[react-devtools/store-test]: fork the test to represent current be… ([hoxyq](https://github.com/hoxyq) in [#29777](#29777)) * Default native inspections config false ([vzaidman](https://github.com/vzaidman) in [#29784](#29784)) * fix[react-devtools] remove native inspection button when it can't be used ([vzaidman](https://github.com/vzaidman) in [#29779](#29779)) * chore[react-devtools]: ip => internal-ip ([hoxyq](https://github.com/hoxyq) in [#29772](#29772)) * Fix #29724: `ip` dependency update for CVE-2024-29415 ([Rekl0w](https://github.com/Rekl0w) in [#29725](#29725)) * cleanup[react-devtools]: remove unused supportsProfiling flag from store config ([hoxyq](https://github.com/hoxyq) in [#29193](#29193)) * [Fiber] Enable Native console.createTask Stacks When Available ([sebmarkbage](https://github.com/sebmarkbage) in [#29223](#29223)) * Move createElement/JSX Warnings into the Renderer ([sebmarkbage](https://github.com/sebmarkbage) in [#29088](#29088)) * Set the current fiber to the source of the error during error reporting ([sebmarkbage](https://github.com/sebmarkbage) in [#29044](#29044)) * Unify ReactFiberCurrentOwner and ReactCurrentFiber ([sebmarkbage](https://github.com/sebmarkbage) in [#29038](#29038)) * Dim `console` calls on additional Effect invocations due to `StrictMode` ([eps1lon](https://github.com/eps1lon) in [#29007](#29007)) * refactor[react-devtools]: rewrite context menus ([hoxyq](https://github.com/hoxyq) in [#29049](#29049))
This lets us expose the component stack to the error reporting that happens here as
console.error
patching. Now if you just callconsole.error
in the error handlers it'll get the component stack added to the end by React DevTools.However, unfortunately this happens a little too late so the Fiber will be disconnected with its
.return
pointer set to null already. So it'll be too late to extract a parent component stack from but you can at least get the stack from source to error boundary. To work around this I manually add the parent component stack in our default handlers when owner stacks are off. We could potentially fix this but you can also just include it yourself if you're callingconsole.error
and it's not a problem for owner stacks.This is not a problem for owner stacks because we'll still have those and so for those just calling
console.error
just works. However, the main feature is that by letting React add them, we can switch to using native error stacks when available.