-
Notifications
You must be signed in to change notification settings - Fork 6
sse: add support of SSE on new clever client #158
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
Conversation
fdcce07 to
1e8cc0d
Compare
1e8cc0d to
e95411d
Compare
7aaaa0b to
1dbd4d6
Compare
florian-sanders-cc
left a comment
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've tested with the examples provided in the docs and it works great 😎
It's very easy to set up and use so really great job 🙌
Didn't see any issue with the docs or the code but I just discovered the subject today so 🤷
|
Ah well, I just remembered: not related to this PR but we could add the |
1dbd4d6 to
90e691b
Compare
90e691b to
0969ad4
Compare
hsablonniere
left a comment
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.
👏 Impressive work @pdesoyres-cc, well done !!
👍 I had to ask Claude some help for the review but the fact that he was able to understand your code really well was a good sign !!
I create a few comments and here are some more general notes:
- I had no autocomplete available for
StreamApplicationRuntimeLogCommandand other commands, I'm not sure why - We may investigate memory leaks with listeners and signals in the future, I'm not sure we really have a problem
- Maybe we could remove all listeners when
.close()is called
- Maybe we could remove all listeners when
- Currently, we only retry on 5xx errors - we could we also retry on 408 (Request Timeout)
or 429 (Too Many Requests)? - What happens if
resume()is called while inconnectingstate? - The debugging is done with console.log, no way to plug a logger, maybe for later...
- Would it be acceptable to use
AbortSignal.any()instead of the custom combine
function?
I'm not sure to understand your problem. I tried, and I have the same behavior than when using
Note that the implementation of Clearly, there are way for improvements.
Yes, maybe later.
AbortSignal.any() is newly available. So we need to wait a little bit |
0969ad4 to
6900dd6
Compare
What this PR do?
This PR adds support for SSE:
mock-apitest toolHow to review