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

feat(helper/streaming): Support Promise<string> or (async) JSX.Element in streamSSE #3344

Merged
merged 2 commits into from
Sep 8, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Aug 31, 2024

#3319

This change allows you to use "hono/jsx" to write the following in streamSSE. I don't think there are many use cases, but the changes on the helper/streaming/sse.ts side are also small and could be added to the implementation.

streamSSE(c, async (stream) => {
  await stream.writeSSE({ data: <div>Hello</div> })
})
streamSSE(c, async (stream) => {
  await stream.writeSSE({
    data: (
      <ErrorBoundary fallback={<div>Error</div>}>
        <AsyncComponent />
      </ErrorBoundary>
    ),
  })
})

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code

Copy link

codecov bot commented Aug 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.76%. Comparing base (040b0d4) to head (935d152).
Report is 16 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3344      +/-   ##
==========================================
- Coverage   95.76%   95.76%   -0.01%     
==========================================
  Files         151      151              
  Lines        9166     9189      +23     
  Branches     2683     2701      +18     
==========================================
+ Hits         8778     8800      +22     
- Misses        388      389       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@usualoma
Copy link
Member Author

@yusukebe @watany-dev @sor4chi
What do you think?

@yusukebe
Copy link
Member

Hey @usualoma

It is interesting that JSX can be written in SSE's data property. With the HTMX SSE extension, the data property will be rendered as HTML. We can write the following code.

import { Hono } from 'hono'
import { streamSSE } from 'hono/streaming'

const app = new Hono()

app.get('/', (c) => {
  return c.html(
    <html>
      <head>
        <script src="https://unpkg.com/[email protected]"></script>
        <script src="https://unpkg.com/[email protected]/sse.js"></script>
      </head>
      <body>
        <div hx-ext="sse" sse-connect="/event-source" sse-swap="EventName"></div>
      </body>
    </html>
  )
})

app.get('/event-source', (c) => {
  return streamSSE(c, async (stream) => {
    while (true) {
      await stream.writeSSE({
        data: (
          <div>
            <b>It's</b> {new Date().toISOString()}
          </div>
        ),
        event: 'EventName'
      })
      await stream.sleep(1000)
    }
  })
})

export default app

I like this feature.

@sor4chi
Copy link
Contributor

sor4chi commented Aug 31, 2024

Hello, @yusukebe and @usualoma.

#1913

I think when I proposed something similar in this PR before, the conclusion was that it was not necessary because it could be done by piping with renderToReadableStream, but what is the difference between this PR and that one?
I'm sorry I haven't fully understood it :)

Is it because you can't use renderToReadableStream with SSE?

@usualoma
Copy link
Member Author

usualoma commented Aug 31, 2024

@sor4chi Thank youuu.
Maybe it's time to re-evaluate #1913!

SSE

Is it because you can't use renderToReadableStream with SSE?

Yes, the main reason is as you pointed out, SSE cannot simply use renderToReadableStream to output. Also, in order to execute the script element output by Suspense, code like the following comment may also be required, and I believe there are many environments where renderToReadableStream is not suitable.

#3319 (comment)

stream

As for the stream referenced in #1913, it can be simply piped with renderToReadableStream. And in general, script elements output by Suspense are also executed without any additional implementation.

However, there are some APPs that can be written more simply by #1913, and considering the consistency with SSE, I feel that it would be better to reopen #1913 and merge it.

(Sorry, I'm submitting this in the middle of a bit of a children's event, but that's the way I see it)

@usualoma
Copy link
Member Author

usualoma commented Sep 1, 2024

We should consider whether or not to re-evaluate #1913, but for now, in this pull request, I refactored the duplicated parts in context.ts and sse.ts so that one line of resolveCallback is all that is needed.

I think this refactoring will also simplify the implementation of #1913.

@sor4chi
Copy link
Contributor

sor4chi commented Sep 2, 2024

Thank you for your answer, I see what you mean.
Let's discuss #1913 after this has been merged!

In the meantime, I'm all for this idea.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

yusukebe commented Sep 5, 2024

@usualoma @sor4chi Thank you!

I'll create a next branch for the next new minor release v4.6.0 later. After that, I'll merg this into it!

@yusukebe yusukebe changed the base branch from main to next September 8, 2024 06:57
@yusukebe yusukebe merged commit 8ca155e into honojs:next Sep 8, 2024
14 checks passed
@yusukebe yusukebe mentioned this pull request Sep 11, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants