Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should Stream be really deprecated? #14

Open
wladpaiva opened this issue Sep 10, 2024 · 3 comments
Open

Should Stream be really deprecated? #14

wladpaiva opened this issue Sep 10, 2024 · 3 comments

Comments

@wladpaiva
Copy link

wladpaiva commented Sep 10, 2024

Nothing wrong with the generator function but doesn't work on every possible case.

If you are sending data from a event emitter like this you won't be able to use the generator

import { Elysia } from "elysia";
import { Stream } from "@elysiajs/stream";
import { EventEmitter } from "node:events";

/**
 * The event emitter for the whole application.
 */
export let emitter = new EventEmitter<{
  x: [];
}>();

const app = new Elysia()
  .get(
    "/",
    () =>
      new Stream(async (stream) => {
        console.log("Started streaming");

        emitter.on("x", () => {
          stream.send(new Date().toISOString());
        });
      })
  )
  .listen(3000);

setInterval(() => {
  console.info("Emitting transaction status changed");
  emitter.emit("x");
}, 30_000);

console.log(
  `🦊 Elysia is running at http://${app.server?.hostname}:${app.server?.port}`
);
@raduconst06
Copy link

raduconst06 commented Sep 16, 2024

I believe the approach from below should work exactly as you need:

import { Elysia } from "elysia";
import { EventEmitter } from "node:events";

/**

  • The event emitter for the whole application.
    */
    export let emitter = new EventEmitter();

const app = new Elysia()
.get(
"/",
async function* () {
console.log("Started streaming");

  try {
    while (true) {
      // Wait for the next "x" event
      await new Promise((resolve) => {
        emitter.once("x", resolve);
      });

      // Yield the data
      yield new Date().toISOString();
    }
  } finally {
    // Cleanup code if necessary
    console.log("Client disconnected, stopping stream.");
  }
}

)
.listen(3000);

setInterval(() => {
console.info("Emitting transaction status changed");
emitter.emit("x");
}, 3_000);

console.log(
🦊 Elysia is running at http://${app.server?.hostname}:${app.server?.port}
);

@wladpaiva
Copy link
Author

Screenshot 2024-09-17 at 11 33 54

Yea, not sure what's advantage and still couldn't make the stream to wait for the event.

Maybe is just the use case tho.
The generator function seems like a clear winner for streams in general but for SSE, the Stream package seems more robust

@guyutongxue
Copy link

The generator handler still does have a critical issue: it doesn't implement text/event-stream protocol, thus yielding object / strings might not be recognizable for SSE frontends. (elysiajs/elysia#741)

A migration guide is use yield `event: message\ndata: ${JSON.stringify(object)}\n\n`; to replace the previous stream.send(object), we should do this until elysiajs/elysia#743 merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants