[Fiber] Track debugInfo on module state instead of stack #29807
Merged
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
Jun 7, 2024
| @@ -1963,7 +1958,12 @@ function createChildReconciler( | |||
| // the error or suspending if needed. | |||
| const throwFiber = createFiberFromThrow(x, returnFiber.mode, lanes); | |||
| throwFiber.return = returnFiber; | |||
| if (__DEV__) { | |||
| throwFiber._debugInfo = currentDebugInfo; | |||
There was a problem hiding this comment.
This is the effective change. We now can pick this up from module scope before clearing it.
|
Comparing: 270229f...25fc6fa 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
trackserversource
branch
from
1df6d56
to
acce1cb
Compare
acdlite
approved these changes
Jun 11, 2024
Notably with DON'T use try/finally because if something throws we want to observe what it was at the time we threw. 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.
sebmarkbage
force-pushed
the
trackserversource
branch
from
acce1cb
to
25fc6fa
Compare
sebmarkbage
added a commit
that referenced
this pull request
Jun 11, 2024
Stacked on #29807. Conceptually the error's owner/task should ideally be captured when the Error constructor is called but neither `console.createTask` does this, nor do we override `Error` to capture our `owner`. So instead, we use the nearest parent as the owner/task of the error. This is usually the same thing when it's thrown from the same async component but not if you await a promise started from a different component/task. Before this stack the "owner" and "task" of a Lazy that errors was the nearest Fiber but if the thing erroring is a Server Component, we need to get that as the owner from the inner most part of debugInfo. To get the Task for that Server Component, we need to expose it on the ReactComponentInfo object. Unfortunately that makes the object not serializable so we need to special case this to exclude it from serialization. It gets restored again on the client. Before (Shell): <img width="813" alt="Screenshot 2024-06-06 at 5 16 20 PM" src="https://github.com/facebook/react/assets/63648/7da2d4c9-539b-494e-ba63-1abdc58ff13c"> After (App): <img width="811" alt="Screenshot 2024-06-08 at 12 29 23 AM" src="https://github.com/facebook/react/assets/63648/dbf40bd7-c24d-4200-81a6-5018bef55f6d">
github-actions bot
pushed a commit
that referenced
this pull request
Jun 11, 2024
Stacked on #29807. Conceptually the error's owner/task should ideally be captured when the Error constructor is called but neither `console.createTask` does this, nor do we override `Error` to capture our `owner`. So instead, we use the nearest parent as the owner/task of the error. This is usually the same thing when it's thrown from the same async component but not if you await a promise started from a different component/task. Before this stack the "owner" and "task" of a Lazy that errors was the nearest Fiber but if the thing erroring is a Server Component, we need to get that as the owner from the inner most part of debugInfo. To get the Task for that Server Component, we need to expose it on the ReactComponentInfo object. Unfortunately that makes the object not serializable so we need to special case this to exclude it from serialization. It gets restored again on the client. Before (Shell): <img width="813" alt="Screenshot 2024-06-06 at 5 16 20 PM" src="https://github.com/facebook/react/assets/63648/7da2d4c9-539b-494e-ba63-1abdc58ff13c"> After (App): <img width="811" alt="Screenshot 2024-06-08 at 12 29 23 AM" src="https://github.com/facebook/react/assets/63648/dbf40bd7-c24d-4200-81a6-5018bef55f6d"> DiffTrain build for commit 383b2a1.
github-actions bot
pushed a commit
that referenced
this pull request
Jun 11, 2024
Stacked on #29807. Conceptually the error's owner/task should ideally be captured when the Error constructor is called but neither `console.createTask` does this, nor do we override `Error` to capture our `owner`. So instead, we use the nearest parent as the owner/task of the error. This is usually the same thing when it's thrown from the same async component but not if you await a promise started from a different component/task. Before this stack the "owner" and "task" of a Lazy that errors was the nearest Fiber but if the thing erroring is a Server Component, we need to get that as the owner from the inner most part of debugInfo. To get the Task for that Server Component, we need to expose it on the ReactComponentInfo object. Unfortunately that makes the object not serializable so we need to special case this to exclude it from serialization. It gets restored again on the client. Before (Shell): <img width="813" alt="Screenshot 2024-06-06 at 5 16 20 PM" src="https://github.com/facebook/react/assets/63648/7da2d4c9-539b-494e-ba63-1abdc58ff13c"> After (App): <img width="811" alt="Screenshot 2024-06-08 at 12 29 23 AM" src="https://github.com/facebook/react/assets/63648/dbf40bd7-c24d-4200-81a6-5018bef55f6d"> DiffTrain build for [383b2a1](383b2a1)
sebmarkbage
added a commit
that referenced
this pull request
Jun 11, 2024
…azy (#29823) Stacked on #29807. This lets the nearest Suspense/Error Boundary handle it even if that boundary is defined by the model itself. It also ensures that when we have an error during serialization of properties, those can be associated with the nearest JSX element and since we have a stack/owner for that element we can use it to point to the source code of that line. We can't track the source of any nested arbitrary objects deeper inside since objects don’t track their stacks but close enough. Ideally we have the property path but we don’t have that right now. We have a partial in the message itself. <img width="813" alt="Screenshot 2024-06-09 at 10 08 27 PM" src="https://github.com/facebook/react/assets/63648/917fbe0c-053c-4204-93db-d68a66e3e874"> Note: The component name (Counter) is lost in the first message because we don't print it in the Task. We use `"use client"` instead because we expect the next stack frame to have the name. We also don't include it in the actual error message because the Server doesn't know the component name yet. Ideally Client References should be able to have a name. If the nearest is a Host Component then we do use the name though. However, it's not actually inside that Component that the error happens it's in App and that points to the right line number. An interesting case is that if something that's actually going to be consumed by the props to a Suspense/Error Boundary or the Client Component that wraps them fails, then it can't be handled by the boundary. However, a counter intuitive case might be when that's on the `children` props. E.g. `<ErrorBoundary>{clientReferenceOrInvalidSerialization}</ErrorBoundary>`. This value can be inspected by the boundary so it's not safe to pass it so if it's errored it is not caught. ## Implementation The first insight is that this is best solved on the Client rather than in the Server because that way it also covers Client References that end up erroring. The key insight is that while we don't have a true stack when using `JSON.parse` and therefore no begin/complete we can still infer these phases for Elements because the first child of an Element is always `'$'` which is also a leaf. In depth first that's our begin phase. When the Element itself completes, we have the complete phase. Anything in between is within the Element. Using this idea I was able to refactor the blocking tracking mechanism to stash the blocked information on `initializingHandler` and then on the way up do we let whatever is nearest handle it - whether that's an Element or the root Chunk. It's kind of like an Algebraic Effect. cc @unstubbable This is something you might want to deep dive into to find more edge cases. I'm sure I've missed something. --------- Co-authored-by: eps1lon <sebastian.silbermann@vercel.com>
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.
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.