robjtede / actix-web-lab

Experimental extractors, middleware, and other extras for possible inclusion in Actix Web.
Apache License 2.0
96 stars 14 forks source link

SSE: Just use Stream instead of SseSender #62

Closed payload closed 11 months ago

payload commented 11 months ago

Looking at the SSE examples, an odd part for me is to use an SSE specific channel call to get a SseSender and use that in a producer. This PR shows that it can be independent of the channel type by sticking to the concept that any Stream trait object can be a producer of server-sent events. So all I need in a producer is something which is Stream eventually.

If an unbounded channel or a different kind of channel is preferred by the user it should be obvious that SSE is not imposing any special requirement on the channel type. It just needs to be Stream eventually.

I view the SSE Responder in this case as a consumer of SSE events and the producer is the user defined thing producing eventually SSE events or things which can be turned into SSE events.

Funnily tokio mpsc bounded Receiver is not implementing Stream itself but tokio-stream provides a ReceiverStream wrapper type which takes the Receiver and implements Stream. Here in the PR I introduced Sse::from_receiver to be a bit more convenient in this regard. The user does not need to use the tokio-stream dependency by himself.

There are multiple Sse::from_* functions now and it is maybe not the best pattern here. I need feedback if maybe another trait and From implementations for this trait and Receiver (and all the other tokio receiver types) are better such that Sse::from can be used in all cases.

After wrapping a Receiver in a ReceiverStream a user could now also call map on it. So it is more composable than receiver types by themself. For example this allows patterns where the data producer is independent of SSE, for example produces a user defined type or String and than the stream is mapped into Sse::Data.

Other feedback to this SSE interface

When the producer is not channels based but a stream by itself, for example a streaming answer from some database query, I can imagine that this is mapped quite well to SSE events. So this is good.

I got confused by the use of Infallible in the examples because I did not knew Infallible. It also feels a bit clunky to produce Results of Events in a stream. It is discouraging when the example needs to use turbofish to say that there are no errors. Ok::<_, Infallible>(sse::Event::Data(data)) Can this be more convenient? Would have expected just Ok(data).

Sse::Event::Comment confused me and I needed to look SSE comments up in MDN to find out that they are actually not showing up on the client side javascript events. I think they don't need to be so prominent in the examples.

Thanks @cjrh for the great discussions on this during #EuroRust23 🦀

robjtede commented 11 months ago

Thanks so much for both your work during the impl workshop!