-
-
Notifications
You must be signed in to change notification settings - Fork 483
feat: sse ping_interval events #4098
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4098 +/- ##
==========================================
- Coverage 98.37% 98.36% -0.01%
==========================================
Files 348 348
Lines 15890 15944 +54
Branches 1755 1762 +7
==========================================
+ Hits 15631 15683 +52
- Misses 122 123 +1
- Partials 137 138 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sobolevn can you help me please with litestar/response/streaming.py:154: error: in mypy. I don't follow why this an error occur.
|
|
This should not be named timeout as it is not a timeout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a different design. Take a look at how the stream handles client disconnects. Ideally, the two background tasks can share the same task group. Otherwise, there's unnecessary overhead involved, and cancellations become quite complex
08d4749 to
6cf631b
Compare
a998181 to
2c7fdd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some minor comments.
We'd need tests for the missing coverage and to test the functionnality too but this looks good overall!
I have read all the SSE tests and don't understand how I can test it. Can you help me, please? I thought I would create an example with ping_interval and test it. |
…event generator is exhausted, updated tests
dff5816 to
174bc0f
Compare
|
@provinzkraut, friendly ping. Can you review, please? I'm moved out work on ping messages to another class |
|
I didnt check in details thechanges but one thing I'd like to see if you dont mind is a little addition in the docs for that feature: https://docs.litestar.dev/latest/usage/responses.html#server-sent-event-responses something like that below this would do the job:
If ping_interval is set to a positive value, blablabla. It is useful for instance on Telegram blablabla, (see the issue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @RenameMe1, unfortunately this still doesn't look just right.
The main issue is the separation of concerns here.
We do have a ASGIStreamingResponse. It should not care about SSE specifics. To implement SSE specific functionality, you'll have to implement it on a separate response class (which you did with ASGIStreamingSSEResponse), and then use that when returning an SSE stream.
|
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4098 |
|
Hello @provinzkraut, I'm moved out SSE implementation into Thanks for you help, and so sorry for mistakes >< |
|
@provinzkraut, friendly ping. Can you review, please? I hope, my work is right |
|
@provinzkraut, friendly ping. Can you review, please? Maybe it would be better to close this PR and let other people do it? |
Description
I would like to propose a solution for issue #4082.
ASGIStreamingSSEResponsewhich supported sending ping messagesCloses
#4082