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

Add juniper_graphql_transport_ws crate for new subscription protocol #1158

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

ccbrown
Copy link
Contributor

@ccbrown ccbrown commented Apr 4, 2023

This adds a new crate: juniper_graphql_transport_ws

This crate implements the new graphql-transport-ws protocol as defined here.

This protocol obsoletes the old graphql-ws protocol. Unfortunately the naming situation is a bit confusing as the old protocol uses the "graphql-ws" WebSocket subprotocol and the new protocol's reference implementation is in a repo named "graphql-ws". But the right way to go to me seems to be to map the WebSocket subprotocols into the crate names.

See #1022 for discussion.

This also adds a serve_graphql_transport_ws function to the juniper_warp crate, which is analogous to serve_graphql_ws. Ideally we would do some auto-negotiation and use the correct protocol intelligently, but it's a little unclear where this should go or what the cleanest way to implement it would be. It is possible to do this in Warp, but Warp doesn't provide first-class support for it. You can read about this exact use-case in seanmonstar/warp#760. I'm punting on that part for a follow-up PR maybe. Same with other juniper_actix changes.

Closes #1022.

@ccbrown ccbrown requested a review from tyranron April 4, 2023 03:11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file started out as a copy/paste from juniper_graphql_ws. The control flow is mostly identical except the new protocol defines several situations in which the server must close the connection with certain codes. So instead of a stream of messages, the connection is a stream of Output::Message(msg) / Output::Close{code, message}.

@ccbrown
Copy link
Contributor Author

ccbrown commented Apr 4, 2023

CI failures are unrelated: #1159

@ccbrown ccbrown force-pushed the graphql-transport-ws-protocol branch from 2eae1c5 to a6ae386 Compare April 4, 2023 22:55
@tyranron
Copy link
Member

@ccbrown first of all, thank you for spending time on this ♥️

Since the control flow doesn't differ much, maybe it's better to implement this in the existing juniper_graphql_ws crate directly, and control the protocol versions via Cargo features?

@ccbrown
Copy link
Contributor Author

ccbrown commented Apr 10, 2023

Since the control flow doesn't differ much, maybe it's better to implement this in the existing juniper_graphql_ws crate directly, and control the protocol versions via Cargo features?

A common use-case is for servers to support both simultaneously and negotiate the correct protocol to use on each connection, as discussed in seanmonstar/warp#760. To support that use-case, we can't force the users into one or the other at compile-time.

I also think the changes, while relatively minor, are pervasive enough that it makes switching between the protocols via conditionals very messy and hard to follow.

@jdeepee
Copy link

jdeepee commented Aug 23, 2023

Dropping a message here to say that we have been using this for the past months in our systems. All queries, mutations and subscriptions seem to be working great, with clients communication to our server using JS & Rust implementations.

Thank you very much for this work, saved us :)

@LegNeato
Copy link
Member

I'm going to merge this as-is, thank you!

@LegNeato LegNeato enabled auto-merge (squash) August 24, 2023 22:51
@LegNeato LegNeato merged commit 27430bf into master Aug 25, 2023
195 of 198 checks passed
@LegNeato LegNeato deleted the graphql-transport-ws-protocol branch August 25, 2023 01:02
@tyranron
Copy link
Member

A common use-case is for servers to support both simultaneously and negotiate the correct protocol to use on each connection, as discussed in seanmonstar/warp#760. To support that use-case, we can't force the users into one or the other at compile-time.

I also think the changes, while relatively minor, are pervasive enough that it makes switching between the protocols via conditionals very messy and hard to follow.

@ccbrown agree. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add graphql-ws support in light of subscriptions-transport-ws deprecation
4 participants