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

WebSocket support for caliban-quick #2150

Merged
merged 12 commits into from
Mar 24, 2024
Merged

WebSocket support for caliban-quick #2150

merged 12 commits into from
Mar 24, 2024

Conversation

kyri-petrou
Copy link
Collaborator

Main changes:

  1. Moved Protocol, WebSocketHooks and StreamTransformer into the core module
  2. Added request handling for WS requests in the caliban-quick module and exposed methods to configure hooks & keep-alive duration
  3. Deprecated the ZHttpAdapter object, signalling that it will be removed in a future release

@kyri-petrou kyri-petrou added the breaking change The PR contains binary incompatible changes label Mar 4, 2024
import zio.stream.ZStream

trait StreamTransformer[-R, +E] {
def transform[R1 <: R, E1 >: E](stream: ZStream[R1, E1, GraphQLWSOutput]): ZStream[R1, E1, GraphQLWSOutput]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m wondering if we could use a ZPipeline for this instead of defining a new type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that would be ideal, although my only concern is that it's a user-facing breaking change.

Having said that, the migration should be very straightforward by simply using ZPipeline.fromFunction so perhaps we should just do it and document the migration

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already breaking right? So as long as we are doing that can probably also move this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not bin-compatible, but other than a deprecation warning the previous changes were source compatible.

Actually I think we might be able to keep it source-compatible (at least for the deprecated trait). Let me give it a go

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I overloaded the message method in the deprecated WebSocketHooks object to accept a StreamTransformer, that way it should be source-compatible for users that don't manually create an instance of WebSocketHooks

case ChannelEvent.UserEventTriggered(HandshakeComplete) =>
out.runForeach(frame => ch.send(ChannelEvent.Read(frame))).forkScoped
case ChannelEvent.Read(WebSocketFrame.Text(text)) =>
ZIO.suspend(queue.offer(readFromString[GraphQLWSInput](text)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulpdaniels @frekw, do you happen to know how should the server handle invalid messages? Based on this logic, we are going to raise an error if the frame cannot be decoded to GraphQLWSInput, which AFAICT will close the stream.

Is this sound? Or should we be ignoring messages we can't decode?

@kyri-petrou
Copy link
Collaborator Author

I'll merge this one in before we start getting merge conflicts, but also so that we have a snapshot version to try out / test.

@kyri-petrou kyri-petrou merged commit 0552909 into series/2.x Mar 24, 2024
10 checks passed
@kyri-petrou kyri-petrou deleted the quick-websockets branch March 24, 2024 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The PR contains binary incompatible changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants