[Fizz] Support abort reasons #24680
[Fizz] Support abort reasons #24680
Conversation
|
Comparing: 7e8a020...cdeeb04 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
|
|
This looks like it affected the file size of |
There was a problem hiding this comment.
AbortController allows a reason to be passed to abort(). We should probably pass that object through and let that be the object that's passed to onError. If it's a string or something with .message, we use that as the error message on the client.
Then we can pass a custom error from renderToString to the abort function.
So FizzServer exports abort(reason) and we call that with signal.reason in the Web Streams form. Node streams abort() can start accepting a reason too.
|
I like that a lot better but there is one thing that bothers me It feels wrong for the reason to be so prescriptive about where it will show up. It doesn't make sense for instance for the renderToString function to pass along as a reason "This Suspense boundary did not finish rendering" however that is the correct wording for the error we eventually want to show up in the client. I solved this by having Fizz append the reason to the error message which is marginally better. But now that I've exposed the ability to send custom reasons in, if someone passes in an Error, i'd have to either give up providing any error guidance in abortTask and just pass the error through, or manipulate the error-like object by reading the message property and reconstructing the error which might hide stack details the implementor would find useful. Additionally, the current implementation restricts reasons to strings or error-like objects. But onError takes mixed and I suppose one should really be allowed to pass in anything for the reason. But this just exacerbates the challenge of what to do about the messaging for each boundary |
There was a problem hiding this comment.
There's some discussion here. My understanding is that the current WHATWG recommendation is to pass the reason straight through and throw whatever it was.
However, Node.js chose not to do that for their APIs and instead attach it to the .cause of the error.
I think the way to think about it is that the client gets a synthetic wrapper error and really the "cause" is the reason on the client. That's the part that's associated with a suspense boundary and why that message matters there is all on the client.
onError doesn't have anything to do with that it's a suspense boundary that didn't finish. That's just that some error happened.
So I think the move here is that onError accepts the raw reason while the serialized error is a synthetic wrapper.
Conceptually it's like:
class SuspenseBoundaryError extends Error {
constructor(cause) {
this.message = `The server did not finish this Suspense boundary: ${cause.message}`;
this.cause = cause;
}
}
function report(reason) {
onError(reason);
reportToClient(new SuspenseBoundaryError(reason));
}
However since we lose the actual instances and classes as part of the serialization anyway. The cause object isn't as important, it's also a fairly new feature anyway.
So I feel like it's fine that we just append something like The server did not finish this Suspense boundary to the message of the reason - but only in the part that gets serialized to the client. It's also just DEV only.
| // This is called to early terminate a request. It puts all pending boundaries in client rendered state. | ||
| export function abort(request: Request): void { | ||
| export function abort(request: Request, reason: ?AbortReason): void { | ||
| request.abortReason = reason; | ||
| try { | ||
| const abortableTasks = request.abortableTasks; | ||
| abortableTasks.forEach(abortTask, request); |
There was a problem hiding this comment.
Instead of stashing it on the request and adding another field, you can just pass it through here (and the same thing inside abortTask). You use a closure instead of the overly clever context argument.
There was a problem hiding this comment.
that was my first instinct. Sometimes I assume there is some 4d chess going on and the avoidance of the closure is paramount but once you point it out it's obvious that it isn't that important. 🤪
There was a problem hiding this comment.
In hot paths that's the right instinct but in this case it's an exceptional case.
| @@ -2158,8 +2175,11 @@ export function startFlowing(request: Request, destination: Destination): void { | |||
| } | |||
| } | |||
|
|
|||
| type AbortReason = string | {message: string, ...}; | |||
There was a problem hiding this comment.
It's a little unclear what the types of these should be but my understanding is that it's moving towards being effectively a throw given these apis:
https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted
As well as similar recommendations to throw the signal.reason. That's not always how built-ins deal with them though - e.g. fetch.
Throws can be anything really (but should typically be Error objects).
Type wise they're mixed though which is why onError accepts mixed. This needs to also accept mixed. So you don't really need this type.
|
In the standard AbortSignal I think you get an Maybe this part can probably be implemented in ReactFizzServer if the reason is |
| @@ -31,10 +31,11 @@ function onError() { | |||
| // Non-fatal errors are ignored. | |||
| } | |||
|
|
|||
| function renderToStringImpl( | |||
| export function renderToStringImpl( | |||
There was a problem hiding this comment.
This file is a build entry point:
https://github.com/facebook/react/blob/main/scripts/rollup/bundles.js#L273
Exporting something here exposes it to UMD builds as well as the CJS builds.
If you want to share an internal helper you have to move this function to a third module that is imported by both.
Fizz supports aborting the render but does not currently accept a reason. The various render functions that use Fizz have some automatic and some user-controlled abort semantics that can be useful to communicate with the running program and users about why an Abort happened. This change implements abort reasons for renderToReadableStream and renderToPipeable stream as well as legacy renderers such as renderToString and related implementations. For AbortController implementations the reason passed to the abort method is forwarded to Fizz and sent to the onError handler. If no reason is provided the AbortController should construct an AbortError DOMException and as a fallback Fizz will generate a similar error in the absense of a reason For pipeable streams, an abort function is returned alongside pipe which already accepted a reason. That reason is now forwarded to Fizz and the implementation described above. For legacy renderers there is no exposed abort functionality but it is used internally and the reasons provided give useful context to, for instance to the fact that Suspsense is not supported in renderToString-like renderers Some notable special case reasons are included below If no reason is provided then a manufactured reason "signal is aborted without reason" If a writable stream errors then a reason "The destination stream errored while writing data." If a writable stream closes early then a reason "The destination stream closed early."
bd66c7b to
20331f1
Compare
b952b61 to
0438912
Compare
| 'error', | ||
| createAbortHandler( | ||
| request, | ||
| // eslint-disable-next-line react-internal/prod-error-codes |
There was a problem hiding this comment.
Any particular reason this should not be minified?
There was a problem hiding this comment.
I’ll be honest, I’m not sure why any errors in fizz should be minified. The code stays in the server so payload is not the most critical thing and it forks the dev/prod behavior of onError which would likely lead to many confused devs not sure why their logic is broken in prod. And we don’t serialize the error and send to the client.
Is there a benefit to minifying that I’m missing?
There was a problem hiding this comment.
Most of the server builds (in particular the Node ones) don't minify but they do it globally for the whole build in bundles.js.
The browser ones do minify, partially because they're actually sometimes used in browsers. In particular renderToString is commonly used (although probably shouldn't) but you can also use streams e.g. in Service Workers.
That said, we should probably be size conscious even for the server because cold-starts on the server is such a significant issue.
So it should probably be up to those settings rather than on a per-error level.
There was a problem hiding this comment.
But what about the runtime divergence? How does one even test that their apps onError logic does what it is supposed to? Some errors are hard to trigger on purpose. I suppose this’ll be a challenge for onRecoverableError in the client. Perhaps a eager-lazy loading of the real error messages could be implemented if and only if you are trying to do logic with them?
There was a problem hiding this comment.
For logging, it's not really different than client-side logging is today. You log the minified error and then you can automatically reverse it from the codes.json in UI (like Sentry does I believe).
I imagine you typically would log them both to the same kind of system anyway.
There was a problem hiding this comment.
For things like branching on semantic logic like "if this was intentionally aborted, do something different" you really shouldn't use the error message anyway. E.g. built-in errors can be translated or change at the browser's will. There's things like .name or instanceof for that.
There was a problem hiding this comment.
As always, you're right :)
| let error = | ||
| reason === undefined | ||
| ? // eslint-disable-next-line react-internal/prod-error-codes | ||
| new Error('signal is aborted without reason') |
There was a problem hiding this comment.
Actually, on a second thought. This is kind of a weird error message in the Node API because there was no signal involved.
scripts/error-codes/codes.json
Outdated
| @@ -419,5 +419,6 @@ | |||
| "431": "React elements are not allowed in ServerContext", | |||
| "432": "This Suspense boundary was aborted by the server.", | |||
| "433": "useId can only be used while React is rendering", | |||
| "434": "`dangerouslySetInnerHTML` does not make sense on <title>." | |||
| "434": "`dangerouslySetInnerHTML` does not make sense on <title>.", | |||
| "435": "The server did not finish this Suspense boundary. The server used \"%s\" which does not support Suspense. If you intended for this Suspense boundary to render the fallback content on the server consider throwing an Error somewhere within the Suspense boundary. If you intended to have the server wait for the suspended component please switch to \"%s\" which supports Suspense on the server" | |||
There was a problem hiding this comment.
I don't see this error anywhere now since these are just passed as strings and not new Error(...) they don't minified.
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
Fizz supports aborting the render but does not currently accept a reason. The various render functions that use Fizz have some automatic and some user-controlled abort semantics that can be useful to communicate with the running program and users about why an Abort happened.
This change implements abort reasons for renderToReadableStream and renderToPipeable stream as well as legacy renderers such as renderToString and related implementations.
For AbortController implementations the reason passed to the abort method is forwarded to Fizz and sent to the onError handler. If no reason is provided the AbortController should construct an AbortError DOMException and as a fallback Fizz will generate a similar error in the absence of a reason
For pipeable streams, an abort function is returned alongside pipe which already accepted a reason. That reason is now forwarded to Fizz and the implementation described above.
For legacy renderers there is no exposed abort functionality but it is used internally and the reasons provided give useful context to, for instance to the fact that Suspense is not supported in renderToString-like renderers
Some notable special case reasons are included below
If no reason is provided then a manufactured reason "signal is aborted without reason"
If a writable stream errors then a reason "The destination stream errored while writing data."
If a writable stream closes early then a reason "The destination stream closed early."