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: implement stream.onAbort #1871

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Conversation

sor4chi
Copy link
Contributor

@sor4chi sor4chi commented Dec 29, 2023

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

related: #1795 #1770

@sor4chi sor4chi closed this Dec 29, 2023
@sor4chi sor4chi reopened this Dec 29, 2023
@sor4chi sor4chi changed the base branch from main to next December 29, 2023 13:42
@sor4chi sor4chi mentioned this pull request Dec 29, 2023
3 tasks
@yusukebe yusukebe added the v3.12 label Dec 31, 2023
@yusukebe
Copy link
Member

yusukebe commented Jan 1, 2024

@sor4chi

Is this ready to review?

@sor4chi
Copy link
Contributor Author

sor4chi commented Jan 1, 2024

Happy new year @yusukebe.
Yes, please.

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

@sor4chi

Happy New Year!

I've checked it and tried to use it in some environment. But is this expected behavior?

Area.mp4

The code:

import { serve } from '@hono/node-server'
import { Hono } from '../../src'
import { streamText } from '../../src/helper/streaming'

export const app = new Hono()

app.get('/', async (c) => {
  return streamText(c, async (stream) => {
    stream.onAbort(async () => {
      console.log('Aborted!')
      await stream.close()
    })
    for (;;) {
      stream.writeln('Hello World!')
      console.log('Hello World!')
      await new Promise((r) => setTimeout(r, 1000))
    }
  })
})

serve(app)

@HeyITGuyFixIt
Copy link
Contributor

@yusukebe in your example, the loop still goes on, but there is no logic to stop the loop after the stream is aborted. You could make it a while loop instead of a for loop, and check for a variable (e.g. while (!aborted)) that is set by the event handler.

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

Hi @HeyITGuyFixIt

So you are saying that it should be code like this? I understand!

app.get('/', async (c) => {
  let aborted = false
  return streamText(c, async (stream) => {
    stream.onAbort(async () => {
      console.log('Aborted!')
      aborted = true
    })
    while (!aborted) {
      stream.writeln('Hello World!')
      console.log('Hello World!')
      await new Promise((r) => setTimeout(r, 1000))
    }
  })
})

@HeyITGuyFixIt
Copy link
Contributor

@yusukebe yeah. Give it a try to make sure that works

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

@HeyITGuyFixIt

It works well! What do you think of this PR? Looks good for you?

If @sor4chi can check again, I'd like to merge this into the next branch.

@sor4chi
Copy link
Contributor Author

sor4chi commented Jan 2, 2024

@yusukebe
I have noticed this behavior, but since the purpose is to take an abort hook in the first place and not to close the stream.

So I'd like to leave it as it is since I think I can handle it by implementing it myself if I want to do it as @HeyITGuyFixIt has presented it.

@HeyITGuyFixIt
Copy link
Contributor

@yusukebe it looks good to me

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

@sor4chi @HeyITGuyFixIt Thanks! Merge now.

@yusukebe yusukebe merged commit 4dfcda8 into honojs:next Jan 2, 2024
22 checks passed
nakasyou pushed a commit to nakasyou/hono that referenced this pull request Jan 13, 2024
* feat: extend `StreamAPI` for response interception and adapt stream handler for it

* chore: denoify
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.

3 participants