apollographql / router

A configurable, high-performance routing runtime for Apollo Federation 🚀
https://www.apollographql.com/docs/router/
Other
785 stars 256 forks source link

Websocket protocol name is misnamed for subscription #5458

Open bnjjj opened 1 month ago

bnjjj commented 1 month ago

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.

michaelcaterisano commented 1 month ago

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.