-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Symmetric ws handler #1061
Symmetric ws handler #1061
Conversation
Tencent integration test seems to be stuck |
That's strange, given there are no integration tests for Tencent (we only have a Placholder object in the test directory) |
Is this likely to be an problem on my end? |
I can reproduce it locally using I didn't have a chance to look too closely, but I can confirm that test is getting stuck, so I suspect it's either a problem in the JavaWebsocketClient or something related to the test setup. |
http4k-realtime-core/src/main/kotlin/org/http4k/websocket/SymmetricWsHandler.kt
Outdated
Show resolved
Hide resolved
http4k-realtime-core/src/main/kotlin/org/http4k/websocket/SymmetricWsHandler.kt
Outdated
Show resolved
Hide resolved
…etricWsHandler.kt Co-authored-by: Oliver Becker <ob@obqo.de>
…etricWsHandler.kt Co-authored-by: Oliver Becker <ob@obqo.de>
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1061 +/- ##
============================================
- Coverage 84.53% 84.51% -0.02%
- Complexity 2014 2016 +2
============================================
Files 579 581 +2
Lines 13241 13263 +22
Branches 1741 1745 +4
============================================
+ Hits 11193 11209 +16
- Misses 1246 1248 +2
- Partials 802 806 +4 ☔ View full report in Codecov by Sentry. |
I fixed my garbage test and the build is passing now. Thanks @s4nchez for finding it for me. |
77c28c1
to
bf32030
Compare
@s4nchez Did you mean to push your server contract changes to this branch? |
@oharaandrew314 no, that was a mistake and I've reverted it. Sorry for the confusion. |
There is a question about if we can make this work more generally - ie. is the more symmetric implementation os WsHandler a better fit long term for http4k. For this, we'd also need to work out if we can mount it onto a server in the same way as the current implementation. This could also apply to SSE as well. |
I originally wanted to make this have no breaking changes at the cost of a
more bloated API. As it is now, I don't think it can support adding a
server.
If we're ok with breaking changes, I can come back with a suggested
solution that would replace the existing WsHandler and be truly symmetric.
…On Sat, Feb 24, 2024, 6:11 a.m. David Denton ***@***.***> wrote:
There is a question about if we can make this work more generally - ie. is
the more symmetric implementation os WsHandler a better fit long term for
http4k. For this, we'd also need to work out if we can mount it onto a
server in the same way as the current implementation.
This could also apply to SSE as well.
—
Reply to this email directly, view it on GitHub
<#1061 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG2JSAVZCSPZXKKWTI2ZNDYVHDH7AVCNFSM6AAAAABDP7ZQZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGMZTAMZVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think we're happy enough to support a breaking change as long as it retains the simplicity and testability. We could start with the new interfaces in the incubator package if we want to put it in incrementally and then promote it at the next major version bump. As long as we can retain the simplicity and testability then it's worth exploring as an option. 😄 |
Ok. I'll close this for now then. |
Closes #1049
You can finally inject a
SymmetricWsHandler
into your application and have it be implemented by a server or a client. This will make testing websocket clients much easier.Supported Websocket clients: