lukeed / fetch-event-stream

A tiny (736b) utility for Server Sent Event (SSE) streaming via `fetch` and Web Streams API
MIT License
376 stars 2 forks source link

Convert `events` to `ReadableStream` ? #1

Open lukeed opened 7 months ago

lukeed commented 7 months ago

Perhaps events() should be a ReadableStream of ServerSentMessages instead of an AsyncIterator.

Practically, the stream could still be used with this syntax:

for await (let event of events(res)) {
  console.log('<<', event.data);
}

The difference would be that a ReadableStream (I think, although am fairly confident) performs better and you have the ability to easily pipe it to some Writer or Transformer stream. For example, imagine:

events(res).pipeThrough(new JSONTransformer).pipeTo(...)

To do the same now, with the AsyncIterator, you have to manually loop the iterable and apply your work; or, convert it to a ReadableStream to be able to do any pipe-work (mentioned above):

// AKA, current implementation
let gen = events(res) as AsyncGenerator<ServerSentMessage, void, unknown>;

// Option 1, where supported
let rs1 = ReadableStream.from(gen);

// Option 2, manual workaround
let rs2 = new ReadableStream({
    async pull(ctrl) {
      let { value, done } = await gen.next();

      if (done) ctrl.close();
      else ctrl.enqueue(value);
    },
  });
}

Considerations

victordidenko commented 7 months ago

Why async iterator in the first place? Why not EventTarget, like built-in EventSource?

lukeed commented 7 months ago

EventTargets aren't composable (like AsyncIterators or ReadableStreams) and so force you to use event-based callback handling, which is fine, but that makes the code/data control flow less obvious. Plus so much of the EventTarget API isn't strictly necessary for parsing or surfacing SSE messages. Most of the code (either mine or polyfill'd) would be to support the EventTarget itself, not the utility.

pngwn commented 6 months ago

ReadableStreams aren't treated as AsyncIterators everywhere yet; see support. As of now, only Deno >= 1.0, Node >= 16.5.0, and Firefox >= 110 support this feature.

Until this is true, I don't think it makes sense to make this change. It seems pretty straightforward to convert this into a ReadableStream, so why not provide a helper function to handle the conversion for this instead?

What about the opposite case, how simple is it to convert a ReadableStream into an AsyncIterator where the protocol is not implemented yet?

lukeed commented 6 months ago

I manually iterate the async iterator now until completion as of now. Works well & bypasses all the compat issues.

In addresses that #3 issue, I had a ReadableStream branch going and it was a bit more code to set up. Not necessarily a deal breaker, however I do think not being able to for await (let x of events) {...} in common envs is more of a deal breaker