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.
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.
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.
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.
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.
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.
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.
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.
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.
Should this be DEV only?
There was a problem hiding this comment.
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.
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.
Does this intermediate object get DCE?
There was a problem hiding this comment.
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.
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.
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.
Yeah I got this to work
gnoff
force-pushed
the
on-recoverable-error
branch
2 times, most recently
from
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.
@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.
gnoff
force-pushed
the
on-recoverable-error
branch
from
9214ca9 to
8b596cd
Compare
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