Provides an EventSource implementation #246

huntc · 2022-08-23T09:57:54Z

Inspired by the WebSocket API.

Example usage:

use gloo_net::eventsource::futures::EventSource;
use wasm_bindgen_futures::spawn_local;
use futures::{stream, StreamExt};

let mut es = EventSource::new("http://api.example.com/ssedemo.php").unwrap();
let stream_1 = es.subscribe("some-event-type").unwrap();
let stream_2 = es.subscribe("another-event-type").unwrap();

spawn_local(async move {
    let mut all_streams = stream::select(stream_1, stream_2);
    while let Some(Ok((event_type, msg))) = all_streams.next().await {
        console_log!(format!("1. {}: {:?}", event_type, msg))
    }
    console_log!("EventSource Closed");
})

TODO:

Find/build an EventSource endpoint for testing
Test from within my real-world application

Fixes ranile/reqwasm#8
Fixes #8

Modelled on the WebSocket API.

hseeberger

For the maintainers of this repository: I am the original author of SSE support in Akka HTTP (see https://github.com/hseeberger/akka-sse) and have been working with Rust in production for some years, also created some OSS libraries (e.g. https://crates.io/crates/pub-sub-client).

Thanks Chris for this excellent PR. I only have some minor suggestions and I'd love to see this being merged.

hseeberger · 2022-09-03T10:19:42Z

crates/net/Cargo.toml

@@ -37,7 +38,7 @@ wasm-bindgen-test = "0.3"
 futures = "0.3"

 [features]
-default = ["json", "websocket", "http"]
+default = ["json", "websocket", "http", "eventsource"]


Generally I am reluctant to putting "too" many features into the default one, but as we already have websocket here I guess we can also have eventsource.

Yeah, seemed to make sense to continue following the convention.

hseeberger · 2022-09-03T10:21:04Z

crates/net/README.md

@@ -19,7 +19,7 @@
 <sub>Built with 🦀🕸 by <a href="https://rustwasm.github.io/">The Rust and WebAssembly Working Group</a></sub>
 </div>

-HTTP requests library for WASM Apps. It provides idiomatic Rust bindings for the `web_sys` `fetch` and `WebSocket` API
+HTTP requests library for WASM Apps. It provides idiomatic Rust bindings for the `web_sys` `EventSource`, `fetch` and `WebSocket` APIs.


Maybe reorder "fetch , EventSource" to sort by specificy/power.

When in doubt, I put things in alphabetical order. Happy to reorder here though.

hseeberger · 2022-09-03T10:32:46Z

crates/net/src/eventsource/futures.rs

+    /// events without an event field as well as events that have the
+    /// specific type `event: message`. It will not trigger on any
+    /// other event type.
+    pub fn subscribe_event(&mut self, event_type: &str) -> Result<(), JsError> {


I suggest to rename this function to just subscribe. And the same for unsubscribe below. It's obvious that we are dealing with events and there is nothing else to subsrcibe to.

huntc · 2022-09-03T12:25:39Z

Commit 1ea4f0a addresses @hseeberger's feedback. Thanks for the review, Heiko!

huntc · 2022-09-03T12:33:01Z

Unsure why the browser tests are failing on CI. Doesn't appear related.

ranile

Why not use gloo_events::EventListener instead of manually registering callbacks?

Also. I'm not sure if this is the best way to expose the API. What if someone wants to subscribe to different events at different places? I would prefer if each subscribe method would return a EventSubscription that can be consumed independently of the EventSource. Of course, this causes complications with closing the underlying web_sys::EventSource. One solution I can think of is to close the connection when all the EventSubscriptions are dropped. A method to forcefully close the connection can also be provided but with a warning that there may be EventSubscriptions that are still using this connection. Waiting on them will lead to a deadlock

pub struct EventSource { ... }

impl EventSource {
    fn new(...) -> Self;
    fn subscribe_event(&self, event: &str) -> EventSubscription;
    fn subscribe_all(&self) -> EventSubscription { self.subscribe_event("message") }
}

struct EventSubscription { ... }
impl EventSubscription {
    // This is our event source. It can Rc'd to be passed around
    fn event_source(&self) -> &EventSource { ... }
}
impl Stream for EventSubscription { ... }
impl Drop for EventSubscription { unsubscribe the event }

crates/net/README.md

crates/net/src/eventsource/futures.rs

crates/net/src/eventsource/mod.rs

huntc · 2022-09-03T13:17:13Z

Why not use gloo_events::EventListener instead of manually registering callbacks?

I'm unsure what the stringified event names should be when registering in a more generic way. I think it might be message, but then I'm unsure how the event type to subscribe to would be used.

How we actually register the event types is an implementation detail though. Would you mind if we defer to an improved method in a subsequent PR if you feel it would benefit the code?

Also. I'm not sure if this is the best way to expose the API. What if someone wants to subscribe to different events at different places? I would prefer if each subscribe method would return a EventSubscription that can be consumed independently of the EventSource. Of course, this causes complications with closing the underlying web_sys::EventSource. One solution I can think of is to close the connection when all the EventSubscriptions are dropped. A method to forcefully close the connection can also be provided but with a warning that there may be EventSubscriptions that are still using this connection. Waiting on them will lead to a deadlock
pub struct EventSource { ... }

impl EventSource {
    fn new(...) -> Self;
    fn subscribe_event(&self, event: &str) -> EventSubscription;
    fn subscribe_all(&self) -> EventSubscription { self.subscribe_event("message") }
}

struct EventSubscription { ... }
impl EventSubscription {
    // This is our event source. It can Rc'd to be passed around
    fn event_source(&self) -> &EventSource { ... }
}
impl Stream for EventSubscription { ... }
impl Drop for EventSubscription { unsubscribe the event }

I think having a stream per subscription adds a bit of complexity, so I'm not sure it is a great idea. In my experience with SSE, I've found that I typically consume all events in one place. However, I appreciate that what you're suggesting is perhaps more in line with the underlying API. Do we really want multiple streams being driven and consumed though? Again, I'm not convinced that the complexity is worth it.

Also, I don't believe there's a way of subscribing to all events. There's an on_message event handler, but I believe that it only fires when there is no event type specified in the source data. If you subscribe to message as an event type then it also yields those events with no event type. Again though, I don't believe you can subscribe to all events.

huntc · 2022-09-03T15:28:03Z

I think having a stream per subscription adds a bit of complexity, so I'm not sure it is a great idea. In my experience with SSE, I've found that I typically consume all events in one place. However, I appreciate that what you're suggesting is perhaps more in line with the underlying API. Do we really want multiple streams being driven and consumed though? Again, I'm not convinced that the complexity is worth it.

I had to scratch this itch to see how it looks. I'm actually quite happy with it and have now pushed a commit that implements what you suggested. The commit is fa096f3.

huntc · 2022-09-03T15:49:48Z

I'm going to revert this back to a draft for now while I get the subscription stream approach working. I'm really unsure about the tests running properly also, so I want to delve into that a bit more.

huntc · 2022-09-03T16:03:31Z

OK, this is ready for review again. I've checked that it continues to work given another app. I'm unhappy that I'm not able to get the tests running locally - well, they run, but I can't seem to get them to fail. I'm also not convinced that the CI issue being reported is related.

Other than that, I'm happy with the approach taken here given the fabulous feedback received.

crates/net/src/eventsource/futures.rs

I had previously assumed that the event source would fire an error event on close, but it only does it on open according to the docs. We therefore fire the error event explicitly so that the subscribed streams know when to shut down.

ranile

Sorry, it took me so long to review this. I've been busy with work and real life stuff.

I'll address the review comments myself and merge it

crates/net/src/eventsource/futures.rs

crates/net/src/eventsource/mod.rs

hseeberger · 2022-09-16T17:12:19Z

Will do thiserror eventually …

huntc · 2022-09-16T20:48:55Z

Sorry, it took me so long to review this. I've been busy with work and real life stuff.

No problem at all! Thanks to you and @hseeberger for the excellent reviews. Hope real life stuff is ok with you.

huntc force-pushed the eventsource branch 2 times, most recently from 29b49ae to ee52f7c Compare August 24, 2022 07:26

Provides an EventSource implementation

c0e7ade

Modelled on the WebSocket API.

huntc force-pushed the eventsource branch from ee52f7c to c0e7ade Compare August 24, 2022 08:10

huntc marked this pull request as ready for review August 26, 2022 09:08

hseeberger approved these changes Sep 3, 2022

View reviewed changes

PR feedback

1ea4f0a

ranile requested changes Sep 3, 2022

View reviewed changes

huntc added 8 commits September 3, 2022 23:20

Removes an unnecessary dependency

69f0b5b

Adds links to MDN

2490c1e

Remove the interior mutability

c23e569

Doc improvement

f815888

Renamed test function to describe what it actually tests

4688b89

We can be sure of the type - no need to check

c2434b4

Allow the lack of copy

6d6cf4f

Streams per subscription

fa096f3

Corrected error handling

0ed9077

huntc marked this pull request as draft September 3, 2022 15:50

huntc marked this pull request as ready for review September 3, 2022 16:01

Permit clone and explain closing behaviour

de45d1f

hseeberger reviewed Sep 4, 2022

View reviewed changes

crates/net/src/eventsource/futures.rs Outdated Show resolved Hide resolved

See that subscribed streams are properly closed

c7e46af

I had previously assumed that the event source would fire an error event on close, but it only does it on open according to the docs. We therefore fire the error event explicitly so that the subscribed streams know when to shut down.

ranile reviewed Sep 16, 2022

View reviewed changes

crates/net/src/eventsource/futures.rs Show resolved Hide resolved

crates/net/src/eventsource/futures.rs Outdated Show resolved Hide resolved

crates/net/src/eventsource/futures.rs Outdated Show resolved Hide resolved

crates/net/src/eventsource/mod.rs Show resolved Hide resolved

Address review comments

79b5dfb

ranile added 2 commits September 16, 2022 22:15

Formatting

f3bb076

Merge branch 'master' into eventsource

aad85b3

ranile approved these changes Sep 16, 2022

View reviewed changes

ranile merged commit ce25de5 into rustwasm:master Sep 16, 2022

huntc deleted the eventsource branch September 16, 2022 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provides an EventSource implementation #246

Provides an EventSource implementation #246

huntc commented Aug 23, 2022 •

edited

hseeberger left a comment

hseeberger Sep 3, 2022

huntc Sep 3, 2022

hseeberger Sep 3, 2022

huntc Sep 3, 2022

hseeberger Sep 3, 2022

huntc Sep 3, 2022

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

ranile left a comment

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

ranile left a comment

hseeberger commented Sep 16, 2022

huntc commented Sep 16, 2022

Provides an EventSource implementation #246

Provides an EventSource implementation #246

Conversation

huntc commented Aug 23, 2022 • edited

hseeberger left a comment

Choose a reason for hiding this comment

hseeberger Sep 3, 2022

Choose a reason for hiding this comment

huntc Sep 3, 2022

Choose a reason for hiding this comment

hseeberger Sep 3, 2022

Choose a reason for hiding this comment

huntc Sep 3, 2022

Choose a reason for hiding this comment

hseeberger Sep 3, 2022

Choose a reason for hiding this comment

huntc Sep 3, 2022

Choose a reason for hiding this comment

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

ranile left a comment

Choose a reason for hiding this comment

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

huntc commented Sep 3, 2022

ranile left a comment

Choose a reason for hiding this comment

hseeberger commented Sep 16, 2022

huntc commented Sep 16, 2022

huntc commented Aug 23, 2022 •

edited