search

brillout / react-streaming

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

impl: Add timeout handling #35

Closed muteor closed 7 months ago

muteor commented 8 months ago

Adding timeout options as per https://react.dev/reference/react-dom/server/renderToReadableStream#aborting-server-rendering and https://react.dev/reference/react-dom/server/renderToPipeableStream#aborting-server-rendering

Follow up of #34.

brillout commented 8 months ago

Nice. How about enabling it by default? I'm not sure what a sensible default number would be 🤔

brillout commented 8 months ago

I wonder whether we should add a onTimeout() callback 🤔

muteor commented 7 months ago

So for defaults, I would probably think something like 20s? That's probably longer than you want but also less than a typical web server/CDN timeout. It's hard to guess peoples workloads so somewhere in the middle might work best.

Can add onTimeout, but do we then want to hide the external abort and signal again? Just thinking it leaves quite large API otherwise meaning there will be more than one way to do the same thing.

brillout commented 7 months ago

So for defaults, I would probably think something like 20s? That's probably longer than you want but also less than a typical web server/CDN timeout. It's hard to guess peoples workloads so somewhere in the middle might work best.

Sounds good.

Can add onTimeout, but do we then want to hide the external abort and signal again? Just thinking it leaves quite large API otherwise meaning there will be more than one way to do the same thing.

I'd agree with keeping the API minimal but I'm thinking there is a valid use case for it, which is when the user has its own cancel mechanism and therefore the user would want to manually control when to abort.

Actually, how about we abstract the signal thing away? In other words: we always return an abort() function.

Also, after thinking more about, I'm not sure I see a use case for onTimeout()? The idea here is that no error is thrown no callback is called (everything seems nomimal) and, instead, React handles the error on his side. Actually there is a use case for it: logging & debugging. It's nice to be able to listen to when the stream times out.

brillout commented 7 months ago

I'm seeing you made some commits; let me know if you're done with the PR.

muteor commented 7 months ago

@brillout Yeah should be good to review.

brillout commented 7 months ago

I made a couple of fixes and improvements. This now LGTM. Let me know if you object with any of my changes or if you see any other potential issues.

muteor commented 7 months ago

yeah looks good!

brillout commented 7 months ago

Released in 0.3.24.

Thanks for the PR. Feel free to take a stab at other contributions!