Unify org.http4s.websocket package, use ByteVector #2143
Merged
Conversation
|
As a concept i totally support this |
|
We should also release a blaze milestone without the old dependency to ensure that we're no longer using it. |
|
LGTM. I agree it was awkward. |
|
whoops, double approval was accident. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
The
org.http4s.websocketpackage is awkwardly split across the http4s-websocket repo and http4s-core. No package should ever live in two artifacts.The reason for the http4s-websocket repo was to share a model between blaze and http4s without adding any dependencies to the former. Since then, the blaze repo dropped websocket support entirely. If we unify it here, we can use a model with all our dependencies. This includes
ByteVector, which is back on our classpath since fs2-core again depends on scodec-bits. That gives us an immutable model.There are all kinds of shady numeric truncations and widenings going on here, and it's worth looking closely at the
WebSocketFrameclass.Additionally, standardizes on the
WebSocketcapitalization in place ofWebsocket