-
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
support errorInfo in onRecoverableError #24591
Conversation
Comparing: 4ddd8b4...3b86e0f 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
|
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 it looks good but would be good to double check with @acdlite.
Let's bikeshed the name of the errorHash
a bit too.
onRecoverableError: (error: mixed) => void, | ||
onRecoverableError: ( | ||
error: mixed, | ||
errorInfo: {errorHash?: ?string, componentStack?: ?string}, |
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.
This is the first place we actually name errorHash
anything in a public API. It's intended to be a hash but you could use it in other ways too (including just returning the full error message in production). I wonder if we should call it something more generic?
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.
errorCode
or maybe errorId
?
It could be a generated ID too. Doesn't need to be a hash.
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 like errorCode
, feels less prescriptive than hash or id. similarly errorDigest
evokes some potentially summarized error data and has analogs with crypto hashing as the output so it works kinda however you use it.
Code perhaps implies codification too much
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.
One downside of errorCode
is that it's often associated with the minified error codes.
Maybe that should actually be a special field too for minified errors so you don't have to parse it yourself. Even for client errors.
(If you don't provide onError on the server and it's a React error, maybe we should use the minified error code as the default "hash" too.)
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.
So
{
// For React errors in Prod Server or Client: the React Error Code
code?: string,
// For Server user errors: onError return value
// For Server React errors if no onError provided: the React Error Code
digest? string, // or whatever this name becomes
// For Dev Server errors (React or user): the server component stack
// For Dev or Prod Client errors (React or user): the client component stack
componentStack?: string,
}
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.
Sounds good. Let's do the code
part later though.
const nextSibling = instance.nextSibling; | ||
let errorMessage /*, errorComponentStack, errorHash*/; | ||
if ( | ||
nextSibling && | ||
nextSibling.nodeType === ELEMENT_NODE && | ||
nextSibling.nodeName.toLowerCase() === 'template' | ||
) { |
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.
You can probably just assume this to be the case. If not, you most likely just get an empty data set.
// if (stack !== null) errorComponentStack = stack; | ||
return { | ||
message: ((nextSibling: any): HTMLTemplateElement).dataset.msg, | ||
stack: ((nextSibling: any): HTMLTemplateElement).dataset.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.
Should this be DEV only?
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's your take on mode divergence on server/client? Should a prod client hitting a dev server not bubble up stacks if they are present?
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.
They should really just always be on the same mode and ideally warn if they're not.
@@ -2662,22 +2675,23 @@ function updateDehydratedSuspenseComponent( | |||
// This boundary is in a permanent fallback state. In this case, we'll never | |||
// get an update and we'll never be able to hydrate the final content. Let's just try the | |||
// client side render instead. | |||
const {errorMessage} = getSuspenseInstanceFallbackErrorDetails( | |||
const {message, hash, stack} = getSuspenseInstanceFallbackErrorDetails( |
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.
Does this intermediate object get DCE?
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 object ErrorDetails returns is not DCE'd but the destructuring does not create an additional object the properties just get assigned directly to local variables.
I considered 3 separate functions to return each part but that seems excessive for an error path. I also considered having ErrorDetails directly return a CapturedValue but that is pushing a new concept into ReactDOMHostConfig and I was unsure if that was wise.
How concerned are you with the object creation in ErrorDetails?
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 you got the tradeoff right. It's just unfortunate that Closure isn't smart enough. Seems like it should be able to.
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.
Maybe if you always returned the same object with the same signature at the end and just assigned to local variables before it?
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 got this to work
c17a9ce
to
553420e
Compare
|
||
// const stack = ((nextSibling: any): HTMLTemplateElement).dataset.stack; | ||
// if (stack !== null) errorComponentStack = stack; | ||
): {digest: ?string, message?: string, stack?: string} { |
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.
@sebmarkbage best way to type functions differently between DEV and prod? I could make it two functions I suppose
errorInfo has been used in Error Boundaries wiht componentDidCatch for a while now. To date this metadata only contained a componentStack. onRecoverableError only receives an error (type mixed) argument and thus providing additional error metadata was not possible without mutating user created mixed objects. This change modifies rootConcurrentErrors rootRecoverableErrors, and hydrationErrors so all expect CapturedValue types. additionally a new factory function allows the creation of CapturedValues from a value plus a hash and stack. In general, client derived CapturedValues will be created using the original function which derives a componentStack from a fiber and server originated CapturedValues will be created using with a passed in hash and optional componentStack.
9214ca9
to
8b596cd
Compare
Summary: This sync includes the following changes: - **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>// - **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>// - **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>// - **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>// - **[a8c9cb18b](facebook/react@a8c9cb18b )**: Land enableSuspenseLayoutEffectSemantics flag ([#24713](facebook/react#24713)) //<Andrew Clark>// - **[a8555c308](facebook/react@a8555c308 )**: [Transition Tracing] Add Tracing Marker Stack ([#24661](facebook/react#24661)) //<Luna Ruan>// - **[8186b1937](facebook/react@8186b1937 )**: Check for infinite update loops even if unmounted ([#24697](facebook/react#24697)) //<Andrew Clark>// - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...256aefb jest_e2e[run_all_tests] Reviewed By: cortinico Differential Revision: D37155957 fbshipit-source-id: 4c0afc95abe8fa13c3803584922c8dc0059ff562
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
errorInfo has been used in Error Boundaries wiht componentDidCatch for a while now. To date this metadata only contained a componentStack. onRecoverableError only receives an error (type mixed) argument and thus providing additional error metadata was not possible without mutating user created mixed objects.
This change modifies rootConcurrentErrors rootRecoverableErrors, and hydrationErrors so all expect CapturedValue types. additionally a new factory function allows the creation of CapturedValues from a value plus a hash and stack.
In general, client derived CapturedValues will be created using the original function which derives a componentStack from a fiber and server originated CapturedValues will be created using with a passed in hash and optional componentStack.
This PR builds on #24551 and once that is merged I will rebase this PR to clean up the diff