[Fizz] Send errors down to client #24551
Conversation
|
Comparing: a276638...c026c26 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
|
| if (errorComponentStack === null) { | ||
| errorComponentStack = | ||
| '[A component stack was not captured at the point of origin of the original error. ' + | ||
| 'This Component stack is from the boundary handling the error.]\n' + |
There was a problem hiding this comment.
The component stacks are parsed. The format is the same as the native stacks also any error parser can do it. This introduces a new part to that string that can break parsers.
It’s not only human readable.
There was a problem hiding this comment.
Ah... good to know. i'll remove. I suppose it's not that useful and should be impossible today
| return writeChunkAndReturn(destination, clientRenderScript2); | ||
| } | ||
|
|
||
| const regexForJSStringsInScripts = /[<\'\"\`\n\u2028\u2029]/g; |
There was a problem hiding this comment.
Is this enough? JSON.stringify does more.
It might be worth doing JSON.stringify first and then escape the extras.
There was a problem hiding this comment.
I could but that would create a string wrapped in extra \" quotes which I'd have to strip out. any downside to adding the missing characters JSON.stringify escapes?
There was a problem hiding this comment.
decided to go the stringify route
| // santizing breaking out of strings and script tags | ||
| case '<': | ||
| return '\\u003c'; | ||
| case "'": |
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
strictly speaking it isn't since we control the wrapping quotations. but if we did a careless quote replacement for our strings and ended up with single quotes for attributes then this would allow escaping. I put it here so that even if we change later the quotes used we won't have to remember to modify the escape procedure
There was a problem hiding this comment.
actually I'm wrong. I think I was mixing up the html attribute context with the js argument one when i was deciding to include these
|
|
||
| const regexForJSStringsInScripts = /[<\'\`\u2028\u2029]/g; | ||
| function escapeJSStringsForInstructionScripts(input: string): string { | ||
| if (typeof input !== 'string') { |
There was a problem hiding this comment.
This check is against an internal value that's already type checked. You can exclude this here.
It would be better to check in inside ReactFizzServer when it first comes from user space code (e.g. check the return value of onRecoverableError) so that the internal type that travels around inside Fizz is sound. It also means that the error can be more specific and we don't have to replicate it in other format configs.
We might do type checks against things like props in here that are unknown before and specific to this host config but this source of the error values aren't specific.
There was a problem hiding this comment.
(e.g. check the return value of
onRecoverableError)
do you mean onError?
It also means that the error can be more specific and we don't have to replicate it in other format configs.
I don't understand what you mean here.
It would be better to check in inside ReactFizzServer when it first comes from user space code
yeah this makes sense to me
| }); | ||
| if (escaped[0] === '"' && escaped[escaped.length - 1] === '"') { | ||
| // This should always be true since JSON.stringify of a string produces a value wrapped in double quotes. | ||
| return escaped.slice(1, -1); |
There was a problem hiding this comment.
Instead of slicing, why don't you just remove these from the clientRenderErrorScriptArgInterstitial etc?
| ), | ||
| serverError | ||
| ? // eslint-disable-next-line react-internal/prod-error-codes | ||
| new Error(serverError) |
There was a problem hiding this comment.
So we're going to do a follow up PR to expand the onRecoverableError API, right? Like add a componentStack?
In that API where does the hash go?
I could imagine for example that the hash goes into the errorInfo object as a second argument:
onRecoverableError(error, {
componentStack: '...',
hash: '...',
});
In that world the error message would just be message. So I think we just make this new Error(errorMessage) for parity with other errors.
The hash could also alternatively go onto this error object itself because it's not applicable to other errors passed to onRecoverableError so the argument on errorInfo isn't necessarily generally applicable.
let error = new Error(errorMessage);
error.hash = errorHash;
There was a problem hiding this comment.
The default error reports (reportError and console.error) don't expose ad hoc properties in the console so attaching them to the error object will not actually expose them unless one implements onRecoverableError and knows to look for them
if we pass a second arg to reportError it is ignored, but if we pass a second arg to console.error (through the defaultOnRecoverableError implementation) we would get a visible rendition of the errorInfo but we would need to stop using reportError as the default handler.
Do you expect frameworks to implement a baseline defualt onRecoverableError handler that knows how to unwrap and utilize these extra properties so that the lack of default exposure is not a concern?
As for using the errorMessage, are you suggesting we use the "there was an error on the server, switched to client rendering" message that we fall back to if no message is provided (in Dev) in prod 100% of the time?
There was a problem hiding this comment.
Do you expect frameworks to implement a baseline defualt onRecoverableError handler that knows how to unwrap and utilize these extra properties so that the lack of default exposure is not a concern?
Yes in particular for the hash. For one you wouldn't have a hash in the first place unless you also implemented onError on the server and the hash is useless to someone that doesn't have a storage for the value that they can look up. You'd have to know to do this. I expect a richer environment to just do that automatically translate the hash to a real error message in the UI for prod.
As for using the errorMessage, are you suggesting we use the "there was an error on the server, switched to client rendering" message that we fall back to if no message is provided (in Dev) in prod 100% of the time?
Yea and in DEV it would be the original message on the server.
Perhaps as a convenience we can also include the hash only in the prod error message similar to what we do with the minified error message.
There was a problem hiding this comment.
Perhaps as a convenience we can also include the hash only in the prod error message similar to what we do with the minified error message.
ah like
`The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering. (Server Error Hash: ${theHash})`
There was a problem hiding this comment.
Yea. I didn’t want to mess with the error message in dev since it’s user provided but for our own it’s fine.
gnoff
force-pushed
the
recoverableErrorServerToClient
branch
2 times, most recently
from
8ba9d5e to
f88fff1
Compare
| } | ||
| return {errorMessage, errorComponentStack, errorHash}; | ||
| } | ||
| export function getSuspenseInstanceFallbackErrorHash( |
There was a problem hiding this comment.
This function is no longer used. Should it be used in DEV? The only reason the other one should be used in prod is because it might contain an error message but only for the one case that maybe should be a hash anyway.
There was a problem hiding this comment.
Hmm I actually removed it but github is showing it in the diff for some reason... have you see that before? Makes it hard to trust diffs...
| @@ -1409,6 +1499,8 @@ function abortTask(task: Task): void { | |||
|
|
|||
| if (!boundary.forceClientRender) { | |||
| boundary.forceClientRender = true; | |||
| boundary.errorMessage = | |||
| 'This Suspense boundary was aborted by the server'; | |||
There was a problem hiding this comment.
For renderToString we send this for every suspense boundary since that works by aborting early.
This seems a little long for production behavior, especially if we've already timed out we might want to keep it short.
I guess the reason we do this is because we don't to treat this as an error and call onError so we don't have a hash for it. Should we treat this as an error though? Maybe you want to know about what parts got aborted.
We could also maybe just assume on the client that if we don't have any hash (like undefined/missing attribute as opposed to empty string) that it was aborted on the server?
There was a problem hiding this comment.
assume on the client that if we don't have any hash ... that it was aborted on the server?
hmm, if you don't use onError on the server the hash is going to be null (currently) and that doesn't get emitted on the template or render instruction. We'd have to distinguish between does not exist (we didn't call onError) and exists but empty (user did not set up onError) and would lead to extra attributes (data-hash) on every non-abort error. This feels fragile too because it would fall apart the moment we had a different kind of error case not requiring a user onError.
maybe there is a different kind of thing like data-sig(nal) that uses an enum for things like abort? it'd make the instruction scripts a bit longer but that's amortized over each client rendered boundary
| } | ||
| if (__DEV__) { | ||
| if (errorComponentStack) { | ||
| (error: any).componentStack = errorComponentStack; |
There was a problem hiding this comment.
We should not add the componentStack this way since we already have a separate protocol for that which is errorInfo.
In this first PR you can just exclude it.
Now that I think about it, it probably makes the most sense to do the same with the hash.
Regardless, let's not expose anything extra to users until we're done with the API.
| @@ -1132,9 +1176,37 @@ function validateIterable(iterable, iteratorFn: Function): void { | |||
| } | |||
| } | |||
|
|
|||
| let renderNodeDestructive = renderNodeDestructiveImpl; | |||
There was a problem hiding this comment.
Don't use mutations in module scope. This messes with the compiler optimizations and resolving of cyclic dependencies because now things start getting dependent on ordering.
Instead create a function that inside of it does the DEV check. It'll get inlined.
| // Currently if a lazy element rejects the component stack will include this | ||
| // special frame. My current intuition is this is good and will aid in identifying | ||
| // where errors are happening. | ||
| pushBuiltInComponentStackInDEV(task, '~lazy-element~'); |
There was a problem hiding this comment.
I believe Fiber calls this 'Lazy'.
react/packages/react-reconciler/src/ReactFiberComponentStack.js
Lines 39 to 40 in c16b005
This Fiber only exists temporarily before we know what the component will be and then it changes tag.
Which is similar to this case.
So I think it makes sense to keep it but call it 'Lazy' instead for parity.
There was a problem hiding this comment.
Yeah maybe I was being pedantic in thinking that distinguishing between Component and elements that are lazy would matter but in reflection it probably would make more sense to the average engineer that any React.Lazy shows up as a Lazy in the stack even if technically it's not in Component position
There was a problem hiding this comment.
Oh, then I misunderstood. I thought this was in the type position.
In that case having no stack frame would align more with the client.
It might be more confusing that you end up with two different errors for the client-side renders and the server-side renders. Like deduping techniques would get confused.
There was a problem hiding this comment.
yeah that make sense
gnoff
force-pushed
the
recoverableErrorServerToClient
branch
2 times, most recently
from
110a01f to
e34e1df
Compare
This commit adds another way to get errors to the suspense instance by encoding them as dataset properties of a template element at the head of the boundary. Previously if there was an error before the boundary flushed there was no way to stream the error to the client because there would never be a client render instruction. Additionally the error is sent in 3 parts 1) error hash - this is always sent (dev or prod) if one is provided 2) error message - Dev only 3) error component stack - Dev only, this now captures the stack at the point of error Another item addressed in this commit is the escaping of potentially unsafe data. all error components are escaped as test for browers when written into the html and as javascript strings when written into a client render instruction.
gnoff
force-pushed
the
recoverableErrorServerToClient
branch
from
e34e1df to
3fc4439
Compare
This is a continuation of @salazarm's PR #24202
Errors are now modeled as
They are sent in html if already known when flushing (e.g. flushing boundary in fallback)
They are also sent in a script in the client render instruction (e.g. a pending boundary rejects and needs to render on client)
Additionally all error content is escaped