search

brillout / react-streaming

React Streaming. Full-fledged & Easy.
MIT License
216 stars 14 forks source link

impl: Add abort pass back for node pipes and options passthrough #34

Closed muteor closed 8 months ago

muteor commented 8 months ago

Quick go at #33

For node pipes added abort to the result, which lets client implement https://react.dev/reference/react-dom/server/renderToPipeableStream#aborting-server-rendering

For both, added passthrough options excluding the used options by the stream wrappers.

brillout commented 8 months ago

Neat, looks good.

The options for Web Stream and Node Stream are quite similar, so I'm inclined to think we should flatten them, so that the user doesn't have to define options twice. That said, we cannot flatten if if there is a situation where the user would want to set a different option dependending on whether the stream is a Web Stream or a Node Stream.

Little nit pick: let's define once the Omit types once and not repeat.

Let's update the docs once we agree on the implementation.

muteor commented 8 months ago

Have updated so we have a single options prop and shared a type for the Omits. Added the new option to the README.

Can squash the commits if you think this is good.

brillout commented 8 months ago

I believe type ReactStreamOptions should be a union not the intersection.

I think we should document the abort return.

brillout commented 8 months ago

Wonderful, thank you for contributing to react-streaming 💚

Released in 0.3.23.

I've been thinking, it would actually be nice if react-streaming implements a timeout that the user can configure / opt-out. WDYT?

muteor commented 8 months ago

Yeah I almost added that but wanted to keep the PR small.

We could probably add an additional option for timeout and call abort/signal.abort internally, can probably keep abort and signal exposed too as maybe there are other cases where those are useful. I also wasn't sure the best way to name the timeout option too, timeout might be a bit generic as its a special case where react doesn't error but just stops further rendering and lets client handle to recovery, maybe renderingTimeout or serverRenderTimeout.

Thanks for the great lib :)

brillout commented 8 months ago

Another PR sounds perfect 👍

I’m glad you like react-streaming :)

brillout commented 8 months ago

As for the naming, I’d argue it is very much a timeout: we cancel the stream after a certain amount of timeout. So I think “timeout” would be a suitable name for the option given the context, and we can document what it does. But maybe I’m missing something and feel free to name it differently. As you wish.