[Fiber] Create virtual Fiber when an error occurs during reconcilation #29804
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
|
Comparing: a0a435d...52ca1a9 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
sebmarkbage
force-pushed
the
trackchildrenerror
branch
from
753d387
to
07b837d
Compare
This lets us rethrow it in the conceptual place of the child.
sebmarkbage
force-pushed
the
trackchildrenerror
branch
from
07b837d
to
52ca1a9
Compare
hoxyq
approved these changes
Jun 11, 2024
acdlite
approved these changes
Jun 11, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Jun 11, 2024
#29804) This lets us rethrow it in the conceptual place of the child. There's currently a problem when we suspend or throw in the child fiber reconciliation phase. This work is done by the parent component, so if it suspends or errors it is as if that component errored or suspended. However, conceptually it's like a child suspended or errored. In theory any thing can throw but it is really mainly due to either `React.lazy` (both in the element.type position and node position), `Thenable`s or the `Thenable`s that make up `AsyncIterable`s. Mainly this happens because a Server Component that errors turns into a `React.lazy`. In practice this means that if you have a Server Component as the direct child of an Error Boundary. Errors inside of it won't be caught. We used to have the same problem with Thenables and Suspense but because it's now always nested inside an inner Offscreen boundary that shields it by being one level nested. However, when we have raw Offscreen (Activity) boundaries they should also be able to catch the suspense if it's in a hidden state so the problem returns. This fixes it for thrown promises but it doesn't fix it for SuspenseException. I'm not sure this is even the right strategy for Suspense though. It kind of relies on the node never actually mounting/committing. It's conceptually a little tricky because the current component can inspect the children and make decisions based on them. Such as SuspenseList. The other thing that this PR tries to address is that it sets the foundation for dealing with error reporting for Server Components that errored. If something client side errors it'll be a stack like Server (DebugInfo) -> Fiber -> Fiber -> Server -> (DebugInfo) -> Fiber. However, all error reporting relies on it eventually terminating into a Fiber that is responsible for the error. To avoid having to fork too much it would be nice if I could create a Fiber to associate with the error so that even a Server component error in this case ultimately terminates in a Fiber. DiffTrain build for commit 270229f.
github-actions bot
pushed a commit
that referenced
this pull request
Jun 11, 2024
#29804) This lets us rethrow it in the conceptual place of the child. There's currently a problem when we suspend or throw in the child fiber reconciliation phase. This work is done by the parent component, so if it suspends or errors it is as if that component errored or suspended. However, conceptually it's like a child suspended or errored. In theory any thing can throw but it is really mainly due to either `React.lazy` (both in the element.type position and node position), `Thenable`s or the `Thenable`s that make up `AsyncIterable`s. Mainly this happens because a Server Component that errors turns into a `React.lazy`. In practice this means that if you have a Server Component as the direct child of an Error Boundary. Errors inside of it won't be caught. We used to have the same problem with Thenables and Suspense but because it's now always nested inside an inner Offscreen boundary that shields it by being one level nested. However, when we have raw Offscreen (Activity) boundaries they should also be able to catch the suspense if it's in a hidden state so the problem returns. This fixes it for thrown promises but it doesn't fix it for SuspenseException. I'm not sure this is even the right strategy for Suspense though. It kind of relies on the node never actually mounting/committing. It's conceptually a little tricky because the current component can inspect the children and make decisions based on them. Such as SuspenseList. The other thing that this PR tries to address is that it sets the foundation for dealing with error reporting for Server Components that errored. If something client side errors it'll be a stack like Server (DebugInfo) -> Fiber -> Fiber -> Server -> (DebugInfo) -> Fiber. However, all error reporting relies on it eventually terminating into a Fiber that is responsible for the error. To avoid having to fork too much it would be nice if I could create a Fiber to associate with the error so that even a Server component error in this case ultimately terminates in a Fiber. DiffTrain build for [270229f](270229f)
sebmarkbage
added a commit
that referenced
this pull request
Jun 11, 2024
Stacked on #29804. Transferring of debugInfo was added in #28286. It represents the parent stack between the current Fiber and into the next Fiber we're about to create. I.e. Server Components in between. ~I don't love passing DEV-only fields as arguments anyway since I don't trust closure to remove unused arguments that way.~ EDIT: Actually it seems like closure handled that just fine before which is why this is no change in prod. Instead, I track it on the module scope. Notably with DON'T use try/finally because if something throws we want to observe what it was at the time we threw. Like the pattern we use many other places. Now we can use this when we create the Throw Fiber to associate the Server Components that were part of the parent stack before this error threw. There by giving us the correct parent stacks at the location that threw.
hoxyq
added a commit
that referenced
this pull request
Jun 18, 2024
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 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.
This lets us rethrow it in the conceptual place of the child.
There's currently a problem when we suspend or throw in the child fiber reconciliation phase. This work is done by the parent component, so if it suspends or errors it is as if that component errored or suspended. However, conceptually it's like a child suspended or errored.
In theory any thing can throw but it is really mainly due to either
React.lazy(both in the element.type position and node position),Thenables or theThenables that make upAsyncIterables.Mainly this happens because a Server Component that errors turns into a
React.lazy. In practice this means that if you have a Server Component as the direct child of an Error Boundary. Errors inside of it won't be caught.We used to have the same problem with Thenables and Suspense but because it's now always nested inside an inner Offscreen boundary that shields it by being one level nested. However, when we have raw Offscreen (Activity) boundaries they should also be able to catch the suspense if it's in a hidden state so the problem returns. This fixes it for thrown promises but it doesn't fix it for SuspenseException. I'm not sure this is even the right strategy for Suspense though. It kind of relies on the node never actually mounting/committing.
It's conceptually a little tricky because the current component can inspect the children and make decisions based on them. Such as SuspenseList.
The other thing that this PR tries to address is that it sets the foundation for dealing with error reporting for Server Components that errored. If something client side errors it'll be a stack like Server (DebugInfo) -> Fiber -> Fiber -> Server -> (DebugInfo) -> Fiber. However, all error reporting relies on it eventually terminating into a Fiber that is responsible for the error. To avoid having to fork too much it would be nice if I could create a Fiber to associate with the error so that even a Server component error in this case ultimately terminates in a Fiber.