Track Stack of JSX Calls #29032
Merged
Track Stack of JSX Calls #29032
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
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
force-pushed
the
ownerstacks
branch
3 times, most recently
from
51e401d
to
d64c7fe
Compare
sebmarkbage
commented
May 9, 2024
| // We use this to trick the filtering of Flight to exclude this frame. | ||
| Object.defineProperty(wrapper, 'name', { | ||
| value: '(<anonymous>)', | ||
| }); |
There was a problem hiding this comment.
This hack wrapper adds another stack frame which I'd rather not include in the assertions in the tests.
I can use this hack in the first phase because so far I'm just asserting on the RSC protocol but this won't work with the client where we won't filter it in React but rather rely on the UI to filter this out.
sebmarkbage
force-pushed
the
ownerstacks
branch
4 times, most recently
from
8103fb3
to
62dbf41
Compare
We track the debug stack using an error. We don't eagerly access the stack to allow it to be lazily generated from the internals. This will be used for passing RSC stacks to the client as well as for user space display. If available, we also track console.createTask separately. This will be used for native stack traces in the native DevTools.
sophiebits
approved these changes
May 9, 2024
| // Extract the stack frame of the callIteratorInDEV function. | ||
| try { | ||
| (callIteratorInDEV: any)({next: null}); | ||
| return ''; |
There was a problem hiding this comment.
nit: should this throw if we can't find the stack? seems like the caller won't do the right thing if this is empty string
(e.g. remove this return and add a throw after the catch)
There was a problem hiding this comment.
I think I need some protection if any of these yield some nonsense or can't be found. Throwing isn't great because it would break the app for non-essential debug information.
github-actions bot
pushed a commit
that referenced
this pull request
May 9, 2024
This is the first step to experimenting with a new type of stack traces behind the `enableOwnerStacks` flag - in DEV only. The idea is to generate stacks that are more like if the JSX was a direct call even though it's actually a lazy call. Not only can you see which exact JSX call line number generated the erroring component but if that's inside an abstraction function, which function called that function and if it's a component, which component generated that component. For this to make sense it really need to be the "owner" stack rather than the parent stack like we do for other component stacks. On one hand it has more precise information but on the other hand it also loses context. For most types of problems the owner stack is the most useful though since it tells you which component rendered this component. The problem with the platform in its current state is that there's two ways to deal with stacks: 1) `new Error().stack` 2) `console.createTask()` The nice thing about `new Error().stack` is that we can extract the frames and piece them together in whatever way we want. That is great for constructing custom UIs like error dialogs. Unfortunately, we can't take custom stacks and set them in the native UIs like Chrome DevTools. The nice thing about `console.createTask()` is that the resulting stacks are natively integrated into the Chrome DevTools in the console and the breakpoint debugger. They also automatically follow source mapping and ignoreLists. The downside is that there's no way to extract the async stack outside the native UI itself so this information cannot be used for custom UIs like errors dialogs. It also means we can't collect this on the server and then pass it to the client for server components. The solution here is that we use both techniques and collect both an `Error` object and a `Task` object for every JSX call. The main concern about this approach is the performance so that's the main thing to test. It's certainly too slow for production but it might also be too slow even for DEV. This first PR doesn't actually use the stacks yet. It just collects them as the first step. The next step is to start utilizing this information in error printing etc. For RSC we pass the stack along across over the wire. This can be concatenated on the client following the owner path to create an owner stack leading back into the server. We'll later use this information to restore fake frames on the client for native integration. Since this information quickly gets pretty heavy if we include all frames, we strip out the top frame. We also strip out everything below the functions that call into user space in the Flight runtime. To do this we need to figure out the frames that represents calling out into user space. The resulting stack is typically just the one frame inside the owner component's JSX callsite. I also eagerly strip out things we expect to be ignoreList:ed anyway - such as `node_modules` and Node.js internals. DiffTrain build for commit 151cce3.
github-actions bot
pushed a commit
that referenced
this pull request
May 9, 2024
This is the first step to experimenting with a new type of stack traces behind the `enableOwnerStacks` flag - in DEV only. The idea is to generate stacks that are more like if the JSX was a direct call even though it's actually a lazy call. Not only can you see which exact JSX call line number generated the erroring component but if that's inside an abstraction function, which function called that function and if it's a component, which component generated that component. For this to make sense it really need to be the "owner" stack rather than the parent stack like we do for other component stacks. On one hand it has more precise information but on the other hand it also loses context. For most types of problems the owner stack is the most useful though since it tells you which component rendered this component. The problem with the platform in its current state is that there's two ways to deal with stacks: 1) `new Error().stack` 2) `console.createTask()` The nice thing about `new Error().stack` is that we can extract the frames and piece them together in whatever way we want. That is great for constructing custom UIs like error dialogs. Unfortunately, we can't take custom stacks and set them in the native UIs like Chrome DevTools. The nice thing about `console.createTask()` is that the resulting stacks are natively integrated into the Chrome DevTools in the console and the breakpoint debugger. They also automatically follow source mapping and ignoreLists. The downside is that there's no way to extract the async stack outside the native UI itself so this information cannot be used for custom UIs like errors dialogs. It also means we can't collect this on the server and then pass it to the client for server components. The solution here is that we use both techniques and collect both an `Error` object and a `Task` object for every JSX call. The main concern about this approach is the performance so that's the main thing to test. It's certainly too slow for production but it might also be too slow even for DEV. This first PR doesn't actually use the stacks yet. It just collects them as the first step. The next step is to start utilizing this information in error printing etc. For RSC we pass the stack along across over the wire. This can be concatenated on the client following the owner path to create an owner stack leading back into the server. We'll later use this information to restore fake frames on the client for native integration. Since this information quickly gets pretty heavy if we include all frames, we strip out the top frame. We also strip out everything below the functions that call into user space in the Flight runtime. To do this we need to figure out the frames that represents calling out into user space. The resulting stack is typically just the one frame inside the owner component's JSX callsite. I also eagerly strip out things we expect to be ignoreList:ed anyway - such as `node_modules` and Node.js internals. DiffTrain build for [151cce3](151cce3)
eps1lon
reviewed
May 17, 2024
| } | ||
| // TODO: We should be freezing the element but currently, we might write into | ||
| // _debugInfo later. We could move it into _store which remains mutable. | ||
| Object.freeze(element.props); |
There was a problem hiding this comment.
On a Next.js client side navigations, we get the following RSC payload:
3:I["(app-pages-browser)/../../../../packages/next/dist/client/components/client-page.js",["app-pages-internals","static/chunks/app-pages-internals.js"],"ClientPageRoot"]
4:I["(app-pages-browser)/./app/[id]/page.tsx",["app/[id]/page","static/chunks/app/%5Bid%5D/page.js"],"default",1]
5:I["(app-pages-browser)/../../../../packages/next/dist/client/components/layout-router.js",["app-pages-internals","static/chunks/app-pages-internals.js"],""]
6:I["(app-pages-browser)/../../../../packages/next/dist/client/components/render-from-template-context.js",["app-pages-internals","static/chunks/app-pages-internals.js"],""]
2:{"name":"","env":"Server","owner":null}
1:D"$2"
8:{"name":"","env":"Server","owner":null}
7:D"$8"
0:["development",[["children",["id","a","d"],[["id","a","d"],{"children":["__PAGE__",{}]}],[["id","a","d"],{"children":["__PAGE__",{},[["$L1",["$","$L3",null,{"props":{"params":{"id":"a"},"searchParams":{}},"Component":"$4"},null]],null],null]},["$","$L5",null,{"parallelRouterKey":"children","segmentPath":["children","$0:1:0:3:0","children"],"error":"$undefined","errorStyles":"$undefined","errorScripts":"$undefined","template":["$","$L6",null,{},null],"templateStyles":"$undefined","templateScripts":"$undefined","notFound":"$undefined","notFoundStyles":"$undefined","styles":null},null],null],[null,"$L7"]]]]
9:"$Sreact.fragment"
7:["$","$9","19VxPb-E-LbHeAfz9ZCJ2",{"children":[["$","meta","0",{"name":"viewport","content":"width=device-width, initial-scale=1"},null],["$","meta","1",{"charSet":"utf-8"},null]]}]
1:null
Which then results in ""Cannot assign to read only property 'Component' of object '#'" thrown from
I don't have a smaller repro yet. Is this a bug with the Flight Server, our usage or should we just not freeze the props on the Flight Client?
There was a problem hiding this comment.
I think I'm seeing a similar issue here hi-ogawa/vite-plugins#318 I don't have a repro outside of my framework yet, but the error seems related to passing client reference as client component props like below:
page.tsx
import { Client1, Client2 } from "./_client";
export default function Page() {
return (
<div>
<h4 className="font-bold">Repro</h4>
{/* TypeError: Cannot assign to read only property 'SomeComp' of object '#<Object>' */}
<Client1 SomeComp={Client2} />
</div>
);
}_client.tsx
"use client";
import React from "react";
export function Client1(props: { SomeComp: React.ComponentType }) {
return (
<div>
[Client1] props.SomeComp = <props.SomeComp />
</div>
);
}
export function Client2() {
return <span>[Client2]</span>;
}I'm using a similar pattern for Error boundary and flight client seems to be throwing for this.
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 is the first step to experimenting with a new type of stack traces behind the
enableOwnerStacksflag - in DEV only.The idea is to generate stacks that are more like if the JSX was a direct call even though it's actually a lazy call. Not only can you see which exact JSX call line number generated the erroring component but if that's inside an abstraction function, which function called that function and if it's a component, which component generated that component. For this to make sense it really need to be the "owner" stack rather than the parent stack like we do for other component stacks. On one hand it has more precise information but on the other hand it also loses context. For most types of problems the owner stack is the most useful though since it tells you which component rendered this component.
The problem with the platform in its current state is that there's two ways to deal with stacks:
new Error().stackconsole.createTask()The nice thing about
new Error().stackis that we can extract the frames and piece them together in whatever way we want. That is great for constructing custom UIs like error dialogs. Unfortunately, we can't take custom stacks and set them in the native UIs like Chrome DevTools.The nice thing about
console.createTask()is that the resulting stacks are natively integrated into the Chrome DevTools in the console and the breakpoint debugger. They also automatically follow source mapping and ignoreLists. The downside is that there's no way to extract the async stack outside the native UI itself so this information cannot be used for custom UIs like errors dialogs. It also means we can't collect this on the server and then pass it to the client for server components.The solution here is that we use both techniques and collect both an
Errorobject and aTaskobject for every JSX call.The main concern about this approach is the performance so that's the main thing to test. It's certainly too slow for production but it might also be too slow even for DEV.
This first PR doesn't actually use the stacks yet. It just collects them as the first step. The next step is to start utilizing this information in error printing etc.
For RSC we pass the stack along across over the wire. This can be concatenated on the client following the owner path to create an owner stack leading back into the server. We'll later use this information to restore fake frames on the client for native integration. Since this information quickly gets pretty heavy if we include all frames, we strip out the top frame. We also strip out everything below the functions that call into user space in the Flight runtime. To do this we need to figure out the frames that represents calling out into user space. The resulting stack is typically just the one frame inside the owner component's JSX callsite. I also eagerly strip out things we expect to be ignoreList:ed anyway - such as
node_modulesand Node.js internals.