Skip to content

Conversation

remorses
Copy link

@remorses remorses commented Jul 20, 2024

Fix #742
Fix #741

@remorses
Copy link
Author

What do you think @SaltyAom is this a breaking change or a bug fix? Previously the response was not following the text/event-stream protocol

@luismanfroni
Copy link

Don't know if it's related, but having a better way to catch Aborts would be really great, instead of having to do something like
request.signal.addEventListener("abort", cb) (which is kinda ugly) inside all request generators handlers for cleanups.
Would be cool an "abort" hook, or something like that.

@remorses
Copy link
Author

remorses commented Jul 29, 2024

@SaltyAom do you need any help with the review?

I am already using this fork in production and it works great. Another problem of the current approach is that most servers won't flush the body until they find a new line, using the right text/event-stream protocol fixes that

Copy link
Member

@SaltyAom SaltyAom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks a lot for your PR.
Would be great if you can help me with these changes and elysiajs/eden#114, after that let's get this merge.

src/handler.ts Outdated
if (init.done) {
if (set) return mapResponse(init.value, set, request)
return mapCompactResponse(init.value, request)
let init
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove try catch in here, handleStream function is already wrapped in try-catch that handle onError already

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the app will crash if you throw an error in a generator before yield, i added a test that reproduces this here: https://github.com/remorses/elysia/blob/932951679c89aef6bad08779b6eb7fe94c30335d/test/response/stream.test.ts#L75

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this crash removing the try catch when aot is false. In aot mode it still crashes.

the app crashes because the error is throws asynchronously while mapResponse is a not async, i think there are many more of these bugs hidden in the code, the ideal solution would be to always await mapResonse in aot too

if (init?.value !== undefined && init?.value !== null)
controller.enqueue(
Buffer.from(
`event: message\ndata: ${JSON.stringify(init.value)}\n\n`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would make sense to check typeof object before using JSON.stringify

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't always call JSON.stringify and the user yields a string that contains 2 new lines the response will break the text/event-stream protocol. If you always call JSON.stringify you can be sure that all the new lines are encoded with \n and put in a single line.

The parsing in Eden also becomes simpler because you know messages are always a JSON encoded string instead of using heuristics.

@kravetsone
Copy link
Contributor

Привет, спасибо большое за твой PR. Было бы здорово, если бы ты помог мне с этими изменениями и elysiajs/eden#114 , после этого давайте сделаем это слияние.

Yay! Thanks for the work)

@remorses remorses requested a review from SaltyAom July 30, 2024 13:12
@HK-SHAO
Copy link

HK-SHAO commented Aug 1, 2024

@SaltyAom @remorses In fact I don't think it should be up to elysia stream to handle exactly how to send the streams of the SSE interface, that should be left to the developer. It should be left to the developer, because exactly how to send each id, event, etc. should be set by the developer.

For example I would like to handle the sent id by itself, it contains some of my rules, for example the id contains a timestamp, server id and a pointer. Or I want messages sent for different events to have different event names. Another example is that I may not need the id and event because ChatGPT's stream response only returns data and does not contain the id and event. there is also a case where I only want to send a single comment. In my opinion, all the above scenarios mean that the generator should only be responsible for sending the data as is, and not have to convert it into an SSE response.

@remorses
Copy link
Author

remorses commented Aug 1, 2024

@HK-SHAO if you want customization you will need to implement it from scratch

app.get('/events', async ({ set }) => {
  set.headers['Content-Type'] = 'text/event-stream'
  set.headers['Cache-Control'] = 'no-cache'

  const stream = new ReadableStream({
    async start(controller) {
      for (let count = 0; count < 10; count++) {
        const event = `data: Event ${count}\n\n`
        controller.enqueue(event)
        await Bun.sleep(1000)
      }
      controller.close()
    }
  })

  return new Response(stream)
})

@kravetsone
Copy link
Contributor

@HK-SHAO if you want customization you will need to implement it from scratch

app.get('/events', async ({ set }) => {
  set.headers['Content-Type'] = 'text/event-stream'
  set.headers['Cache-Control'] = 'no-cache'

  const stream = new ReadableStream({
    async start(controller) {
      for (let count = 0; count < 10; count++) {
        const event = `data: Event ${count}\n\n`
        controller.enqueue(event)
        await Bun.sleep(1000)
      }
      controller.close()
    }
  })

  return new Response(stream)
})

Yeah i agree. Elysia should folows specs but if you need sometning custom you can do it

@remorses
Copy link
Author

remorses commented Feb 2, 2025

I created a full framework where async generators have first class support and use SSE, it's called Spiceflow

@SaltyAom
Copy link
Member

I think I have to added input to this for the record

The problem is there are both use case for both text/event-stream and non text/event-stream
eg. stream a response in which response is not a text eg. image, video or create a proxy from AI provider like OpenAI, Claude, etc which are using direct streaming not text/event-stream, or create a proxy from fetch (Elysia can return fetch(url) and it will just become a proxy which is possible because of direct streaming)
I personally haven't come up with an idiomatic API yet which is why I leave it as it until then because I'm afraid of introducing a breaking changes.

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

Successfully merging this pull request may close these issues.

Async generator handler should follow text/event-stream protocol Async generator yielding an object results in [object Object] output
5 participants