[Flight] Parse Stack on the Server and Transfer Structured Stack #30410
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
force-pushed
the
structuredstacks
branch
from
81586f6
to
4d2866b
Compare
sebmarkbage
force-pushed
the
structuredstacks
branch
from
4d2866b
to
1b805b9
Compare
This was referenced Jul 22, 2024
eps1lon
approved these changes
Jul 22, 2024
| @@ -178,18 +178,29 @@ export type Awaited<T> = T extends null | void | |||
| : T // argument was not an object | |||
| : T; // non-thenable | |||
|
|
|||
| export type ReactCallSite = [ | |||
| string, // function name | |||
| string, // file name TODO: model nested eval locations as nested arrays | |||
There was a problem hiding this comment.
I figured the model could maybe be:
export type ReactCallSite = [
string, // function name
string | ReactCallSite, // file name or eval
number, // line
number, // column
]Where if it's a nested callsite that implies it's eval at that location.
sebmarkbage
force-pushed
the
structuredstacks
branch
from
1b805b9
to
6243c28
Compare
All servers currently use the V8 format. Including Bun, which overrides the JSC format to V8 format it seems. However, we might need to configure this for the FB build. Conceptually it's also just something that is environment specific. I now apply filtering on the result which needs to update some hacks.
sebmarkbage
force-pushed
the
structuredstacks
branch
from
6243c28
to
ec47303
Compare
sebmarkbage
added a commit
that referenced
this pull request
Jul 22, 2024
Stacked on #30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: #30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in #30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
sebmarkbage
added a commit
that referenced
this pull request
Jul 22, 2024
…rialized (#30416) Stacked on #30410. If we've parsed another RSC stream on the server from a different RSC server, while using `findSourceMapURL`, the Flight Client ends up adding a `rsc://React/` prefix and a numeric suffix to the URL. It's a virtual file that represents the virtual eval:ed frame in that environment. If we then see that same stack again, we'd serialize a virtual frame to another virtual. Meaning `findSourceMapURL` on the client would see the virtual frame of the intermediate server and it would have to strip it to figure out what source map to use. This PR strips it in the Server if we see a virtual frame. At each new client it always refers to the original stack. We don't have to do this. We could leave it to each `findSourceMapURL` implementation and `captureOwnerStack` parser to recursively strip each layer. It could maybe be useful to have the environment name in the virtual frame to know which server to look for the source map in.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
…rialized (facebook#30416) Stacked on facebook#30410. If we've parsed another RSC stream on the server from a different RSC server, while using `findSourceMapURL`, the Flight Client ends up adding a `rsc://React/` prefix and a numeric suffix to the URL. It's a virtual file that represents the virtual eval:ed frame in that environment. If we then see that same stack again, we'd serialize a virtual frame to another virtual. Meaning `findSourceMapURL` on the client would see the virtual frame of the intermediate server and it would have to strip it to figure out what source map to use. This PR strips it in the Server if we see a virtual frame. At each new client it always refers to the original stack. We don't have to do this. We could leave it to each `findSourceMapURL` implementation and `captureOwnerStack` parser to recursively strip each layer. It could maybe be useful to have the environment name in the virtual frame to know which server to look for the source map in.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: facebook#30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in facebook#30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
…ebook#30410) Stacked on facebook#30401. Previously we were transferring the original V8 stack trace string to the client and then parsing it there. However, really the server is the one that knows what format it is and it should be able to vary by server environment. We also don't use the raw string anymore (at least not in enableOwnerStacks). We always create the native Error stacks. The string also made it unclear which environment it is and it was tempting to just use it as is. Instead I parse it on the server and make it a structured stack in the transfer format. It also makes it clear that it needs to be formatted in the current environment before presented.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
…rialized (facebook#30416) Stacked on facebook#30410. If we've parsed another RSC stream on the server from a different RSC server, while using `findSourceMapURL`, the Flight Client ends up adding a `rsc://React/` prefix and a numeric suffix to the URL. It's a virtual file that represents the virtual eval:ed frame in that environment. If we then see that same stack again, we'd serialize a virtual frame to another virtual. Meaning `findSourceMapURL` on the client would see the virtual frame of the intermediate server and it would have to strip it to figure out what source map to use. This PR strips it in the Server if we see a virtual frame. At each new client it always refers to the original stack. We don't have to do this. We could leave it to each `findSourceMapURL` implementation and `captureOwnerStack` parser to recursively strip each layer. It could maybe be useful to have the environment name in the virtual frame to know which server to look for the source map in.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
Stacked on facebook#30410. Use "owner stacks" as the appended component stack if it is available on the Fiber. This will only be available if the enableOwnerStacks flag is on. Otherwise it fallback to parent stacks. In prod, there's no owner so it's never added there. I was going back and forth on whether to inject essentially `captureOwnerStack` as part of the DevTools hooks or replicate the implementation but decided to replicate the implementation. The DevTools needs all the same information from internals to implement owner views elsewhere in the UI anyway so we're not saving anything in terms of the scope of internals. Additionally, we really need this information for non-current components as well like "rendered by" views of the currently selected component. It can also be useful if we need to change the format after the fact like we did for parent stacks in: facebook#30289 Injecting the implementation would lock us into specifics both in terms of what the core needs to provide and what the DevTools can use. The implementation depends on the technique used in facebook#30369 which tags frames to strip out with `react-stack-bottom-frame`. That's how the implementation knows how to materialize the error if it hasn't already. Firefox: <img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM" src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d"> Follow up: One thing about this view is that it doesn't include the current actual synchronous stack. When I used to append these I would include both the real current stack and the owner stack. That's because the owner stack doesn't include the name of the currently executing component. I'll probably inject the current stack too in addition to the owner stack. This is similar to how native Async Stacks are basically just appended onto the current stack rather than its own.
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
…ebook#30410) Stacked on facebook#30401. Previously we were transferring the original V8 stack trace string to the client and then parsing it there. However, really the server is the one that knows what format it is and it should be able to vary by server environment. We also don't use the raw string anymore (at least not in enableOwnerStacks). We always create the native Error stacks. The string also made it unclear which environment it is and it was tempting to just use it as is. Instead I parse it on the server and make it a structured stack in the transfer format. It also makes it clear that it needs to be formatted in the current environment before presented.
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 #30401.
Previously we were transferring the original V8 stack trace string to the client and then parsing it there. However, really the server is the one that knows what format it is and it should be able to vary by server environment.
We also don't use the raw string anymore (at least not in enableOwnerStacks). We always create the native Error stacks.
The string also made it unclear which environment it is and it was tempting to just use it as is.
Instead I parse it on the server and make it a structured stack in the transfer format. It also makes it clear that it needs to be formatted in the current environment before presented.