Accept `onError()` and return `allReady`

[XiNiHa]

I'm not sure that the behavior of allReady is identical between implementations, but I did my best in reading React code to match the behavior :D

[brillout]

I did my best in reading React code to match the behavior :D

Neat :-).

This is the idea of how react-streaming does error handling:

import { renderToStream } from 'react-streaming/server'

try {
  await renderToStream(<Page />)
  // If we are here, the page shell was successfully rendered.
  // We can send the page shell to the client with a `200` status code. Because,
  // if an error occurs later, React will try to fix the problem on the client-side.
} catch(err) {
  // Error happened during page shell rendering
}

// This is equivalent of doing `await allReady`:
try {
  await renderToStream(<Page />, { disable: true })
  // If we are here, the page was completely and successfully rendererd.
} catch(err) {
  // Error during page rendering
}

Doesn't that work for you?

react-streaming takes care of the whole await allReady thingy. No need for the user to do it.

[XiNiHa]

1. allReady is useful to check if the stream has ended.
2. AFAIK some fatal errors can still happen after shell render ended, notably React internal errors. Users may want to handle them.

[brillout]

1. Use case?
2. React internal errors cannot be catched by react-streaming. I don't think we can do anything about these. These are (very?) rare so I think it's best to just assume them to not happen. (React does have bugs but these are usually on a conceptual level and don't materialize into a throw.)

[XiNiHa]

1. I shoud use this to wait until providing the pageContext. While I should eventually move to streaming the data to get advantages of selective hydration, I want this as a temporary workaround. There could be more edge usecases like this 🤔
2. At least that's the route provided by React used for passing fatal errors, according to the source code. I believe it's better to provide it to users to handle the error just in case (otherwise Node will crash, at least you should allReady.catch(() => {}) internally then), although I'm also not sure what the error will be.

[brillout]

1. Ok makes sense.

2. So it means that React passes potential React bugs as an Error object to the onError() option of renderToPipeableStream({ onError }) and renderToReadableStream({ onError })? Shouldn't React bugs destroy the stream? Isn't that a more canonical way of handling errors during a stream than having a onError() callback? Personally, that's what I would expect.

[XiNiHa]

Actually, I was talking about allReady's reject conditions. onError receives all the errors (both fatal and non-fatal, Suspense-caught) and it can be used for things like logging.

[brillout]

How about this: if React has a bug then react-streaming destroys the stream with the React error, and rejects allReady with some generic error new Error('Stream Error. The stream error is available at `destroy(err)`.') resolves allReady as if it was successful.

resolves allReady as if it was successful.

Since we are changing semantics, maybe we should rename allReady to streamEnd.

No need for a new options.onError() then.

[XiNiHa]

As I said before, onError can also be used for handling errors in Suspense boundaries (and that's the actual main purpose). Therefore it's different from the allReady semantics. I believe both should be added with the same behavior matching each React APIs.

[brillout]

Doesn't React swallow Suspense boundary errors on the server-side?

Isn't the idea that React swallows boundary errors and re-tries to resolve the suspense on the client-side?

Basically: React takes care of boundary errors and there is nothing for the user to do (other than handling the client-side suspense error, e.g. with a Error Boundary).

In other words: all the user has to do to handle Suspense errors is to wrap his root component with an Error Boundary.

Maybe I'm mistaken, but that's what I understood so far.

[XiNiHa]

Yeah that's right and it's more close to logging purpose. Or maybe some additional handling (showing an alert about the error? disabling caching for this response?) could be done.

[brillout]

showing an alert about the error disabling caching for this response logging purpose

If react-streaming destroys the stream, then the user can as well implement these use cases.

Bottom line: isn't it more idiomatic to destroy the stream with the error (i.e. writable.destroy(err)), rather than using a onError() callback?

[brillout]

Actually, we shouldn't destroy the stream upon a Suspense boundary error.

So maybe this then:

• If React bug => destroy the stream.
• If boundary error => call onBoundaryError(). We make it clear in the docs that this is optional and mostly about logging (or sending different headers).

[XiNiHa]

Then we should implement error filtering between onBoundaryError andonError, since onError is called on every error, including fatal and boundary errors. Personally I'd prefer to have just plain onError.

For the stream, it should indeed be destroyed when React error occurs. However I see no reason to bother rejecting allReady with the error, since that matches the renderToReadableStream().allReady's behavior. So no need to wrap it here then.

[brillout]

Then we should implement error filtering between onBoundaryError andonError

Is that possible in a reliable way?

Personally I'd prefer to have just plain onError.

The reason I'm reluctant with having a single onError() callback is that the user will handle error types differently.

A boundary error can safely be ignored because it will be re-tried on the client-side. If the user has error tracking already installed on the client-side, then he may not need to do anything about boundary errors on the server-side.

A React bug is, as you say, fatal and needs quite some complex consideration to handle properly. I'm expecting most users to just ignore potential React bugs and just assume React to be bug-free. This is not perfect, but handling React bugs is really complex: the stream can be corrupted in all kinds of ways and there is no clear way how to recover from this.

So I expect 95% of the users to ignore potential React bugs.

Boundary errors and React bugs will be handled quite differently and I'm therefore leaning towards having two different callbacks.

Or, better yet (from my perspective), to just destroy the stream for React bugs.

So no need to wrap it here then.

My thinking is that it's not the user that handles the stream error.

The writable that is being destroyed is the (Express.js/Node.js/Fastify/...) server in the case of Node.js. How does these server framework handle the situation when res is destroyed? I don't know, but if they are properly implemented then they should error out just like they normally do.

As for Cloudflare Workers, I'm expecting it to error out as usual when the readable is destroyed.

Because 95% of users will not care about React bugs, I think, so far, these default server behaviors to be fine. But only for now: if at some point in the future, when react-streaming starts to get more users, a user stumbles upon a React bug and realizes that Express.js handles that situation really poorly, then we can start thinking of making the user's life easier.

So I'm proposing to just destroy the stream and later see if we need a callback for React bugs.

For the stream, it should indeed be destroyed when React error occurs. However I see no reason to bother rejecting allReady with the error, since that matches the renderToReadableStream().allReady's behavior.

The problem with rejecting streamEnd/allReady is that it forces the user to handle errors. We don't want to force the user to handle React bugs nor boundary errors.

95% of the users will ignore React bugs. And many(/most?) users will as well ignore boundary errors on the server-side (most will probably just wrap their roor component with an error boundary).

So server-side error handling should be opt-in, not opt-out.

More concretly.

Users who ignore boundary errors on the server-side will not want to have a rejected streamEnd/allReady.

For error tracking, onBoundaryError() is easy than try-catch streamEnd/allReady.

For sending other headers, then, yea, rejecting streamEnd/allReady would be more convenient, but only slightly.

If we never reject allReady and never resolve a value for it, I'd suggest to rename it to streamEnd.

[XiNiHa]

Is that possible in a reliable way?

I don't think so (with what I have roughly in mind, it should rely on implementation details of React, which is not great), and that's one of the reasons why I'm against making it onBoundaryError instead of onError.

So I'm proposing to just destroy the stream and later see if we need a callback for React bugs.

I also agree that the stream must be destroyed in this case.

The problem with rejecting streamEnd/allReady is that it forces the user to handle errors. We don't want to force the user to handle React bugs nor boundary errors.

Then let's just make const streamEnd = allReady.catch(() => {}) and return it. It sounds reasonable enough to me 👍

After you make decisions on onError, I'll push the changes.

[brillout]

I'd suggest we go for the hackish solution and open an issue on React's main repo. We can warn the user about it and link the React issue.

Let me quickly create the React issue.

[brillout]

Let me quickly create the React issue.

(Or you can do it if you want; it's quite fun to interact with the React team :-).)

[XiNiHa]

Since I'm less convinced that it's an "issue", I think it's better to do that yourself :D

[brillout]

Yea I'm seeing the issue template is quite restrictive 😅. It's not a bug but I do think it's an issue. Let me create this.

[brillout]

https://github.com/facebook/react/issues/24536

[XiNiHa]

Haven't tested 💩

[brillout]

Which part didn't you test?

[brillout]

Actually, I'm thinking the hack is quite reliable. This is exactly how I would have expected React's stream to behave.

[brillout]

Review done. (Sorry for not having used GitHub's review thingy 😁.)

[XiNiHa]

Well, I think I should change streamEnd to resolve after the whole stream (including react-streaming's contents) ends

[XiNiHa]

I guess it's now in the good state

[brillout]

I made some changes: https://github.com/brillout/react-streaming/pull/2/commits/d07b8cb054fb871f6bc4c7292fc07b4ecff0d524.

Thoughts?

In particular, I've removed the if (!disable) branch for resolving the page shell: https://github.com/brillout/react-streaming/pull/2/files#diff-6f7fc48a66de15bcfb2153f65dd7c57182ef62cbcd25a82654b281f5a891283fL95-R103.

[XiNiHa]

I like the changes! Great.

By the way, the error handling docs should be fixed as it's impossible to send headers after shell render 😏

[brillout]

By the way, the error handling docs should be fixed as it's impossible to send headers after shell render 😏

That's surprising. So what does browsers do when they receive headers e.g. a cache header during the stream? Wasn't there a plan to send Resource Hints Headers during the stream? I believe to have read this in the React 18 Working Group, IIRC.

[XiNiHa]

It's impossible to send headers from the server side after the shell is streamed (since the shell is at the body, which is at after headers) so it errors. I'm not sure about resource hints.

[brillout]

Released 0.2.3

[brillout]

https://stackoverflow.com/questions/72204848/how-to-send-http-headers-during-after-http-body-stream-is-there-spec-work-on-th