-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Include server component names in the componentStack in DEV #28415
Conversation
96254ae
to
6feacf8
Compare
Comparing: 66c8346...fe343f8 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
|
6feacf8
to
78aaedb
Compare
Should the server component frames indicate they are not from the local environment? |
@@ -86,4 +86,41 @@ describe('component stack', () => { | |||
'\n in Example (at **)', | |||
); | |||
}); | |||
|
|||
// @reactVersion >=16.9 |
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.
// @reactVersion >=16.9 |
I think its fair to test this behaviour against react
from main, not against react
17 or 18.
They have a This is actually a little tricky in real life too though because for example error-stack-parser doesn't know how to parse these lines. However, that's already a problem with the built ins and host component frames. So using a regular off-the-shelf parser might not work - which was kind of the point that this should work. Maybe that's just inherent to naive parsers like that though and it's up to them to be a bit smarter. I was considering using brackets instead but I'm not sure if that's used by any formats for other purposes. Where as |
78aaedb
to
fe343f8
Compare
I'm a bit ambivalent about this one because it's not the main strategy that I plan on pursuing. I plan on replacing most DEV-only specific stacks like `console.error` stacks with a new take on owner stacks and native stacks. The future owner stacks may or may not be exposed to error boundaries in DEV but if they are they'd be a new errorInfo property since they're owner based and not available in prod. The use case in `console.error` mostly goes away in the future so this PR is mainly for error boundaries. It doesn't hurt to have it in there while I'm working on the better stacks though. The `componentStack` property exposed to error boundaries is more like production behavior similar to `new Error().stack` (which even in DEV won't ever expose owner stacks because `console.createTask` doesn't affect these). I'm not sure it's worth adding server components in DEV (this PR) because then you have forked behavior between dev and prod. However, since even in the future there won't be any other place to get the *parent* stack, maybe this can be useful information even if it's only dev. We could expose a third property on errorInfo that's DEV only and parent stack but including server components. That doesn't seem worth it over just having the stack differ in dev and prod. I don't plan on adding line/column number to these particular stacks. A follow up could be to add this to Fizz prerender too but only in DEV. DiffTrain build for [8fb0233](8fb0233)
### React upstream changes - facebook/react#28438 - facebook/react#28436 - facebook/react#25954 - facebook/react#28434 - facebook/react#28433 - facebook/react#28432 - facebook/react#28415 - facebook/react#27903 - facebook/react#28430 - facebook/react#28424 - facebook/react#28400 - facebook/react#28422 - facebook/react#28423 - facebook/react#28412 - facebook/react#28418 - facebook/react#28421 - facebook/react#28417 - facebook/react#28399 - facebook/react#28408 - facebook/react#28350 - facebook/react#28387 - facebook/react#28403 - facebook/react#28384 - facebook/react#28409 - facebook/react#28398 - facebook/react#28405 - facebook/react#28328 - facebook/react#28402 - facebook/react#28386 - facebook/react#28388 - facebook/react#28379 - facebook/react#28383 - facebook/react#28390 - facebook/react#28389 - facebook/react#28382 - facebook/react#28348 Closes NEXT-2600
* feat[devtools]: symbolicate source for inspected element ([hoxyq](https://github.com/hoxyq) in [#28471](#28471)) * refactor[devtools]: lazily define source for fiber based on component stacks ([hoxyq](https://github.com/hoxyq) in [#28351](#28351)) * fix[devtools/tree/element]: onClick -> onMouseDown to handle first click correctly ([hoxyq](https://github.com/hoxyq) in [#28486](#28486)) * [DOM] disable legacy mode behind flag ([gnoff](https://github.com/gnoff) in [#28468](#28468)) * Fix Broken Links In Documentation ([justindhillon](https://github.com/justindhillon) in [#28321](#28321)) * Update /link URLs to react.dev ([rickhanlonii](https://github.com/rickhanlonii) in [#28477](#28477)) * [tests] add support for @GATE pragma ([gnoff](https://github.com/gnoff) in [#28479](#28479)) * Devtools: Unwrap Promise in useFormState ([eps1lon](https://github.com/eps1lon) in [#28319](#28319)) * Add support for rendering BigInt ([eps1lon](https://github.com/eps1lon) in [#24580](#24580)) * Include server component names in the componentStack in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [#28415](#28415))
…#28415) I'm a bit ambivalent about this one because it's not the main strategy that I plan on pursuing. I plan on replacing most DEV-only specific stacks like `console.error` stacks with a new take on owner stacks and native stacks. The future owner stacks may or may not be exposed to error boundaries in DEV but if they are they'd be a new errorInfo property since they're owner based and not available in prod. The use case in `console.error` mostly goes away in the future so this PR is mainly for error boundaries. It doesn't hurt to have it in there while I'm working on the better stacks though. The `componentStack` property exposed to error boundaries is more like production behavior similar to `new Error().stack` (which even in DEV won't ever expose owner stacks because `console.createTask` doesn't affect these). I'm not sure it's worth adding server components in DEV (this PR) because then you have forked behavior between dev and prod. However, since even in the future there won't be any other place to get the *parent* stack, maybe this can be useful information even if it's only dev. We could expose a third property on errorInfo that's DEV only and parent stack but including server components. That doesn't seem worth it over just having the stack differ in dev and prod. I don't plan on adding line/column number to these particular stacks. A follow up could be to add this to Fizz prerender too but only in DEV.
* feat[devtools]: symbolicate source for inspected element ([hoxyq](https://github.com/hoxyq) in [facebook#28471](facebook#28471)) * refactor[devtools]: lazily define source for fiber based on component stacks ([hoxyq](https://github.com/hoxyq) in [facebook#28351](facebook#28351)) * fix[devtools/tree/element]: onClick -> onMouseDown to handle first click correctly ([hoxyq](https://github.com/hoxyq) in [facebook#28486](facebook#28486)) * [DOM] disable legacy mode behind flag ([gnoff](https://github.com/gnoff) in [facebook#28468](facebook#28468)) * Fix Broken Links In Documentation ([justindhillon](https://github.com/justindhillon) in [facebook#28321](facebook#28321)) * Update /link URLs to react.dev ([rickhanlonii](https://github.com/rickhanlonii) in [facebook#28477](facebook#28477)) * [tests] add support for @GATE pragma ([gnoff](https://github.com/gnoff) in [facebook#28479](facebook#28479)) * Devtools: Unwrap Promise in useFormState ([eps1lon](https://github.com/eps1lon) in [facebook#28319](facebook#28319)) * Add support for rendering BigInt ([eps1lon](https://github.com/eps1lon) in [facebook#24580](facebook#24580)) * Include server component names in the componentStack in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [facebook#28415](facebook#28415))
I'm a bit ambivalent about this one because it's not the main strategy that I plan on pursuing. I plan on replacing most DEV-only specific stacks like `console.error` stacks with a new take on owner stacks and native stacks. The future owner stacks may or may not be exposed to error boundaries in DEV but if they are they'd be a new errorInfo property since they're owner based and not available in prod. The use case in `console.error` mostly goes away in the future so this PR is mainly for error boundaries. It doesn't hurt to have it in there while I'm working on the better stacks though. The `componentStack` property exposed to error boundaries is more like production behavior similar to `new Error().stack` (which even in DEV won't ever expose owner stacks because `console.createTask` doesn't affect these). I'm not sure it's worth adding server components in DEV (this PR) because then you have forked behavior between dev and prod. However, since even in the future there won't be any other place to get the *parent* stack, maybe this can be useful information even if it's only dev. We could expose a third property on errorInfo that's DEV only and parent stack but including server components. That doesn't seem worth it over just having the stack differ in dev and prod. I don't plan on adding line/column number to these particular stacks. A follow up could be to add this to Fizz prerender too but only in DEV. DiffTrain build for commit 8fb0233.
I'm a bit ambivalent about this one because it's not the main strategy that I plan on pursuing. I plan on replacing most DEV-only specific stacks like
console.error
stacks with a new take on owner stacks and native stacks. The future owner stacks may or may not be exposed to error boundaries in DEV but if they are they'd be a new errorInfo property since they're owner based and not available in prod.The use case in
console.error
mostly goes away in the future so this PR is mainly for error boundaries. It doesn't hurt to have it in there while I'm working on the better stacks though.The
componentStack
property exposed to error boundaries is more like production behavior similar tonew Error().stack
(which even in DEV won't ever expose owner stacks becauseconsole.createTask
doesn't affect these). I'm not sure it's worth adding server components in DEV (this PR) because then you have forked behavior between dev and prod.However, since even in the future there won't be any other place to get the parent stack, maybe this can be useful information even if it's only dev. We could expose a third property on errorInfo that's DEV only and parent stack but including server components. That doesn't seem worth it over just having the stack differ in dev and prod.
I don't plan on adding line/column number to these particular stacks.
A follow up could be to add this to Fizz prerender too but only in DEV.