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 protocol name is misnamed for subscription #5458

Open
bnjjj opened this issue Jun 14, 2024 · 1 comment
Open

Websocket protocol name is misnamed for subscription #5458

bnjjj opened this issue Jun 14, 2024 · 1 comment
Labels
component/subscriptions Pertaining to GraphQL Subscriptions in the Router

Comments

@bnjjj
Copy link
Contributor

bnjjj commented Jun 14, 2024

We're using protocol name for websocket in subscriptions. But for the old one instead of naming it graphql-transport-ws we should name it subscriptions-transport-ws to avoid any confusions.

@bnjjj bnjjj added the component/subscriptions Pertaining to GraphQL Subscriptions in the Router label Jun 14, 2024
@michaelcaterisano
Copy link

michaelcaterisano commented Jun 17, 2024

I don't think this is just a naming issue, the docs seem to be wrong here:

graphql_ws

Used by the graphql-ws library
This subprotocol is the default value and is recommended for GraphQL server libraries implementing WebSocket-based subscriptions.

graphql_transport_ws

Legacy subprotocol used by the subscriptions-transport-ws library, which is unmaintained but provided for backward compatibility.

The sentence "used by the graphql-ws" library seems to imply that the graphql_ws config option is referring to the library's subprotocol. This is wrong; the graphql-ws library uses the graphql-transport-ws subprotocol, which we can see in its docs. In fact, the graphql-ws library was originally named graphql-transport-ws, which is how I think this happened.

The older subscriptions-transport-ws library seems to use a subprotocol named graphql-ws (which is not referring to the library above, since it was not created yet) based on this file in its codebase referring to a protocol named graphql-ws, and this Elixir implementation of subscriptions-transport-ws, whose docs say to set the subprotocol on the server to graphql-ws.

It looks like these associations are correct in the dropdown in Apollo Studio:

image

Naming aside, the current state of the router subscription config is this:

  1. setting the router config protocol to graphql_ws results in a Sec-Websocket-Protocol request header sent from router to graphql-transport-ws. This sort of makes sense since this is the subprotocol used by the graphql-ws library, so we can say that the config is referring to the library here and not its subprotocol.

  2. Setting the router config protocol to graphql_transport_ws results in a Sec-Websocket-Protocol request header value of graphql-ws. This does not make sense; graphql_transport_ws is a protocol, so setting the value of the request header to the name of its parent library is likely to cause bugs and confusion. Renaming this config value to subscription_transport_ws seems reasonable-ish as long as it's clear that its subprotocol is graphql-ws and the expected value of the header will be graphql-ws. This is not currently clear in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/subscriptions Pertaining to GraphQL Subscriptions in the Router
Projects
None yet
Development

No branches or pull requests

2 participants