Interoperability suggestions

[morris]

Hi!

First of all thanks a lot for taking over this and moving GraphQL over web sockets forward, I believe it's quite important to get some consistency here 🎉 !

I was playing around with this a bit and built an alternate server implementation of the protocol, with the goal of supporting @apollo/client out of the box, with no changes on the client side. I found that there's only a few changes that would allow the latest @apollo/client:

• Allow both graphql-transport-ws and graphql-ws sub-protocols (the latter is used with @apollo/client)
• Allow type: "start" in addition to type: "subscribe" for Subscribe messages
• Allow type: "stop" in addition to type: "complete" for client-to-server Complete messages
• Use type: "data" instead of type: "next" for Next messages if socket.protocol === "graphql-ws"
• Allow the client to send Subscribe messages before receiving ConnectionAck. This was the "hardest" one, as the server needs to defer handling the received Subscribe messages until it actually acknowledges the connection. I resolved this through introducing a promise for the acknowledgment, which the Subscribe handlers await before proceeding.

I believe some degree of legacy protocol handling could drive adoption a lot. If we had a server-side GraphQL-over-WS implementation that caters to all popular clients right now, that would be really cool (and graphql-ws could be just that!).

This does not necessarily have to be reflected in the spec and may just go to the implementation. Still, I could imagine adding something like:

• Servers MAY accept Subscribe messages before a client received the ConnectionAck message in order to support legacy clients.
• Servers MUST NOT handle Subscribe messages before sending the ConnectionAck message.
• etc.

I did write an alternate implementation from scratch (and this was quite easy thanks to your work on the protocol) but I could easily provide a PR for the above if you're interested. Let know what you think :)

All the best
Morris

[enisdenjo]

You're very welcome! I am always happy to hear that my work helps. 😄

Given the fact that the client presents supported sub-protocols on the initial WebSocket handshake - the server can choose where to delegate the request (old vs new protocol).

Keeping this in mind, I dont plan on "polluting" a fresh protocol with superfluous additions of an outdated one (same for this reference implementation graphql-ws).

Sorry if this is not the answer you expected... I am fully aware of the transitioning phase, backwards compatibility and the, somewhat, rocky road to adaptation - but changing a working system "for convenience only" is not what I intend to do.

Furthermore, even the main readme of subscriptions-transport-ws recommends transitioning to this lib.

A proper solution is doing a PR to Apollo's stack integrating support for the new protocol and allowing backwards compatibility to the old one.

[morris]

Thanks for your response - totally understand and agree with your reasoning about the spec! Especially since subscriptions-transport-ws was never a standard to begin with, it should in no way hold back defining a future-proof and secure spec.

My line of thought was more a practical one. I believe these simple user stories hold:

• Server implementors want to support as many GraphQL WebSocket agents (and versions thereof) as possible.
• Client implementors want to use whatever agent fits their stack, without server-specific modifications (e.g. the modifications described in the graphql-ws README).

Currently, the only viable transition for these users I see is

1. to install both graphql-ws and subscriptions-transport-ws on the server-side and splitting by the sub-protocol selected by client agents (as you suggested),
2. ask clients to switch to graphql-ws-based agents or have them implement server-specific modifications,
3. and keep subscriptions-transport-ws on server-side as long as there are (relevant) non-complying clients.

That's totally feasible, but obviously less than ideal in terms of effort. I believe for currently live projects (1.) may not be avoidable as legacy clients might have locally optimized against subscriptions-transport-ws in subtle ways and you'll not want to invite WebSocket-sub-protocol-level problems by switching libraries.

However, even in new projects, as a server implementor I will still have to do (1.) in order to support the numerous GraphQL agents in the ecosystem - or force the clients to use the tricks from the graphql-ws README. If I had a server-side library at hand that supported both protocols, and possibly even fixed some issues with subscription-transport-ws, that would reduce our combined invest in this significantly.

Anyways, I just wanted to report that graphql-ws could be this library with some simple modifications, not at all that it should. After all, if nothing breaks, this might actually slow down adoption, as there's no need to transition. (I don't like this thought as I'm an interoperability fan, but it could be true nonetheless.)

We could have a separate, legacy-friendly library once the protocol is finalized, so I guess the more important work is to review the protocol in as much detail as possible and make it somewhat standard soon 🚀

Thanks again for your hard work, I'll be looking out for future developments for sure!

[enisdenjo]

Its much less work to support both transports in userland, than integrating interoperability here and maintaining that additional complexity introduced in favour of laziness.

I don't plan on fixing anything subscriptions-transport-ws (not on their side, not on mine, not ever), and following the abundance of open issues and PRs, seems like no one is... With this, it is not worth supporting both transports - you may want to add backwards compatibility only in favour of easing the transition of a hot system. Nothing else.

In my humble opinion, if you decide to support both in new projects: you're doing it wrong!

Furthermore, a minimal legacy-friendly library boils down to 60 lines of code (you can find the Recipe in the readme too):

import https from 'https';
import ws from 'ws'; // yarn add ws
import { execute, subscribe } from 'graphql';
import { GRAPHQL_TRANSPORT_WS_PROTOCOL } from 'graphql-ws';
import { useServer } from 'graphql-ws/lib/use/ws';
import { SubscriptionServer, GRAPHQL_WS } from 'subscriptions-transport-ws';
import { schema } from 'my-graphql-schema';

// graphql-ws
const graphqlWs = new ws.Server({ noServer: true });
useServer(
  {
    schema,
    execute,
    subscribe,
  },
  graphqlWs,
);

// subscriptions-transport-ws
const subTransWs = new ws.Server({ noServer: true });
SubscriptionServer.create(
  {
    schema,
    execute,
    subscribe,
  },
  subTransWs,
);

// create https server
const server = https.createServer(function weServeSocketsOnly(_, res) {
  res.writeHead(404);
  res.end();
});

// listen for upgrades and delegate requests according to the WS subprotocol
server.on('upgrade', (req, socket, head) => {
  // extract websocket subprotocol from header
  const protocol = req.headers['sec-websocket-protocol'];
  const protocols = Array.isArray(protocol)
    ? protocol
    : protocol?.split(',').map((p) => p.trim());

  // decide which websocket server to use
  const wss =
    protocols?.includes(GRAPHQL_WS) && // subscriptions-transport-ws subprotocol
    !protocols.includes(GRAPHQL_TRANSPORT_WS_PROTOCOL) // graphql-ws subprotocol
      ? subTransWs
      : // graphql-ws will welcome its own subprotocol and
        // gracefully reject invalid ones. if the client supports
        // both transports, graphql-ws will prevail
        graphqlWs;
  wss.handleUpgrade(req, socket, head, (ws) => {
    wss.emit('connection', ws, req);
  });
});

[enisdenjo]

Closing due to inactivity.