-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Reference docs for experimental captureOwnerStack
#7427
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size changes📦 Next.js Bundle Analysis for react-devThis analysis was generated by the Next.js Bundle Analysis action. 🤖 One Page Changed SizeThe following page changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
4a08a62
to
4bf159b
Compare
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.
I think the naming we should go with is:
- Component Stack: what you're calling "parent component stack" here
- Owner Stack: what you're calling "owner component stack" here
* **optional** `onCaughtError`: Callback called when React catches an error in an Error Boundary. Called with the `error` caught by the Error Boundary, and an `errorInfo` object containing the `componentStack`. | ||
* **optional** `onUncaughtError`: Callback called when an error is thrown and not caught by an Error Boundary. Called with the `error` that was thrown, and an `errorInfo` object containing the `componentStack`. | ||
* **optional** `onRecoverableError`: Callback called when React automatically recovers from errors. Called with an `error` React throws, and an `errorInfo` object containing the `componentStack`. Some recoverable errors may include the original error cause as `error.cause`. | ||
* **optional** `onCaughtError`: Callback called when React catches an error in an Error Boundary. Called with the `error` caught by the Error Boundary, and an `errorInfo` object containing the parent Component stack in `componentStack`. |
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.
What is a "parent Component Stack" isn't that just the Component Stack? Adding "parent" makes it sound like it doesn't include the component that errored.
This page should also add a usage item for calling captureOwnerStack
to get the owner stack.
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.
I feel like expanding the existing usages is sufficient. Is that what you meant? Adding a whole new section feels like overkill.
onCaughtError: (error, errorInfo) => { | ||
if (process.env.NODE_ENV !== 'production') { | ||
const ownerStack = captureOwnerStack(); | ||
error.stack += ownerStack; |
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.
Do we really recommend appending it to the existing stack? Won't that add it to the end?
IMO this should be like:
if (__DEV__) {
showDialog(error, ownerStack, componentStack);
} else {
logProdError(error, componentStack);
}
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.
We do that in Next.js and it's conceptually how you arrive at the error if every JSX callsite would just be a function call.
Aren't we getting rid of parent stacks entirely eventually? I thought the plan was for owner stacks to subsume them. In which case I might still discuss which stack frames you'll see but not emphasize the difference so much. |
Not for production logging. They're the ones passed to the deriveStateFromError/componentDidCatch and onError on the server. They only go away in the standard DEV console.error reporting. |
There should also be runable sandboxes for the Usage examples |
To expand on that — it’s not just for it to be nice but because it’s a proof the API actually works end to end and does what the docs page says it does. We’ve had cases where that wasn’t the case and the Usage examples helped uncover the misunderstandings. |
d6e8d90
to
4f59d78
Compare
4f59d78
to
265786e
Compare
265786e
to
47e7e41
Compare
`captureOwnerStack` was called outside of development builds. | ||
For performance reasons, React will not keep track of Owners in production. |
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.
`captureOwnerStack` was called outside of development builds. | |
For performance reasons, React will not keep track of Owners in production. | |
`captureOwnerStack` was called outside of development builds. For performance reasons, React will not keep track of Owner Stacks in production. |
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.
Done
The call of `captureOwnerStack` happened outside of a React controlled function e.g. in a `setTimeout` callback. | ||
During render, Effects, Events, and React error handlers (e.g. `hydrateRoot#options.onCaughtError`) Owner Stacks should be available. |
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.
The call of `captureOwnerStack` happened outside of a React controlled function e.g. in a `setTimeout` callback. | |
During render, Effects, Events, and React error handlers (e.g. `hydrateRoot#options.onCaughtError`) Owner Stacks should be available. | |
The call of `captureOwnerStack` happened outside of a React controlled function e.g. in a `setTimeout` callback. During render, Effects, Events, and React error handlers (e.g. `hydrateRoot#options.onCaughtError`) Owner Stacks should be available. |
And can we add a sandbox with the issue and fix?
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.
Added one for custom DOM event handlers which is also a likely pitfall.
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.
The other comments are nits, but I think there's a usage section still missing here for patching console.error. If you only instrument owner stacks for DEV dialogs in the root error handlers, you will miss owner stacks in console.error calls. So you need to patch console.error, but you need to patch it after dev tools, which is tricky to get right and needs docs.
|
||
// The stacks are logged instead of showing them in the UI directly to highlight that browsers will apply sourcemaps to the logged stacks. | ||
// Note that sourcemapping is only applied in the real browser console not in the fake one displayed on this page. | ||
console.error('Uncaught', error); |
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.
If you've patched console.error
to add owner stacks, and you call console.error here, it will result in multiple owner stacks being applied. It's not clear to me whether you should only patch console.error, or if you should instrument the root handlers and console.error without them overlapping somehow.
@sebmarkbage how should this work ideally? Maybe we're over-emphasizing the root handler case?
The way I expected this to work is like:
onUncaughtError() {
if (process.env.NODE_ENV !== 'production') {
const ownerStack = captureOwnerStack();
sendDevOnlyLogs(error, ownerStack);
} else {
sendProductionLogs(error);
}
// Patch or DevTools will add logs to the console output.
console.error(error);
}
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.
Yeah I think the usage in error handlers is the better example. If there's a need to document polyfilling what React Devtools is doing, we can add it later. You only need to patch console if React Devtools is not available. Otherwise you'll get them either in the last arg or via createTask
(whichever is available).
Co-authored-by: Ricky <[email protected]>
parent Component stack -> Component Stack owner Component stack -> Owner Stack
Was just rephrasing what we already had and gave wrong impression
i.e. everywhere I found mention of `"componentStack"`
c44e0ad
to
1bbf3b8
Compare
Only double newlines usually end up as a newline in rendered markdown.
It's just a (bad) polyfill for what RDT is doing. Better to lean on RDT for owner stacks in console calls.
1bbf3b8
to
86d7eac
Compare
Based on #7430
Preview:
captureOwnerStack
createRoot error handlers
hydrateRoot error handlers
The owner stack vs parent section probably deserves its own page since owner stacks also appear in devtools and
console.createTask
.Should I just move this into a "debugging" section inside "learn"?