Subscription support for GraphiQL

[junminstorage]

Recreated based on #436

See https://github.com/junminstorage/graphql-tutorial for its usage. Specifically see how to start web socket server to support subscription: https://github.com/junminstorage/graphql-tutorial/blob/master/src/index.js

Ready for review.

[junminstorage]

Ping @acao and @danielrearden for reviewing this PR.

[junminstorage]

I will wait for the review comments before solving the conflicts.

[nileio]

is there any update about this PR please ?

[cannc4]

Looking forward to this!

[ghost]

Hi, @junminstorage @acao @danielrearden what is the current status of this PR?

[junminstorage]

Ping @acao @danielrearden again to start the code review so we can move forward.

[acao]

looking great! can you add a section to the readme that shows a simple example implementation with subscriptions? or maybe even add an “examples” directory with your example repo? just so it’s apparent to users how to implement this server side?

we should clarify that this won’t work with lambda or other function as a service platforms.

[junminstorage]

@acao Please take one more look.

[junminstorage]

" or maybe even add an “examples” directory with your example repo? "
My example repo is a full-fledged graphql tutorial-type repo. I need to simplify the sample code over there, translate its JS code to TS, which requires more time. I can do it in next PR if it is useful.

[junminstorage]

Ping @achao for review this PR.

[a-tokyo]

@achao Can you please review this? 🚀 Thanks a bunch!

[hongbo-miao]

Nice job! Just add my two cents. My concern is this will add a library graphiql-subscriptions-fetcher which is archived, not sure it is a good idea.

Also, there are some new simpler and lighter libraries to add GraphQL subscriptions such as graphql-transport-ws which did better job than the heavy subscriptions-transport-ws in my experience.

[a-tokyo]

@hongbo-miao Awesome find! I found graphql-transport-ws much better as well. atm I am using to implement subs and they are working like a charm. (except for a hack that needs to be done.)

@junminstorage, can we maybe use graphql-transport-ws instead? 🚀

Example using graphql-transport-ws with express:

/**
 * Adaptor between our httpServer and gql Subscription server
 *
 * This gets socket.io to work alongside gqls
 * In order to support more than one websocket on the same server, we need to wrap httpServer so that ws doesn't immediately grab the socket and then passes it to SubscriptionServer, which in turn immediately aborts the handshake if it isn't a graphql subscription.
 *
 * more info: https://github.com/apollographql/subscriptions-transport-ws/issues/751#issuecomment-674420442
 */
const _adaptServerToGqls = (
  server: Server,
  gqlsPath?: string = GRAPHQL_ENDPOINT,
): Server => {
  const gqlsServerProxy = new EventEmitter();
  server.on('listening', gqlsServerProxy.emit.bind(gqlsServerProxy));
  server.on('error', gqlsServerProxy.emit.bind(gqlsServerProxy));
  server.on('upgrade', (req: Request, ...rest: Array<any>): void => {
    /** Only upgrade the requests that belong to gql subscription server */
    if (req.url === gqlsPath) {
      gqlsServerProxy.emit('upgrade', req, ...rest);
    }
  });
  return gqlsServerProxy;
};

/**
 * Creates the Apollo Subscription Server for graphql subscriptions
 */
const createSubscriptionServer = () =>
  new SubscriptionServer(
    {
      execute,
      subscribe,
      schema,
      /** optional interval in ms to send KEEPALIVE messages to all clients */
      keepAlive: GQLS_KEEP_ALIVE ? Number(GQLS_KEEP_ALIVE) : 25000,
      onConnect: (
        connectionParams: Object,
        webSocket: WebSocket,
        context: ConnectionContext,
      ) =>
        subscriptionMiddleware(
          null,
          connectionParams,
          webSocket,
          context,
        )(() => {
          /** Reject non-authenticated/public connections */
          if (!webSocket.upgradeReq.user) {
            throw new Error('Authentication required for websocket connection');
          }
          return Promise.resolve({
            ...connectionParams,
            context: {
              rootValue: { request: webSocket.upgradeReq, response: {} },
            },
          });
        }),
      onOperation: (message, params, webSocket, context) =>
        subscriptionMiddleware(
          message,
          params,
          webSocket,
          context,
        )(() =>
          Promise.resolve({
            ...params,
            context: {
              rootValue: { request: webSocket?.upgradeReq, response: {} },
            },
          }),
        ),
      onDisconnect: () => {
        logger.debug('GQLS disconnect');
      },
    },
    {
      server: _adaptServerToGqls(httpServer, GRAPHQL_ENDPOINT),
      path: GRAPHQL_ENDPOINT,
    },
  );

/** Runs after the main http server starts listening */
const postListen = () => {
  subscriptionServer = createSubscriptionServer();
};

[hongbo-miao]

@a-tokyo hmm, I think you posted wrong demo.. For graphql-transport-ws, it is just simple like

import { createServer } from 'graphql-transport-ws';
import { buildSchema, execute, subscribe } from 'graphql';

const schema = buildSchema(`
  type Query {
    hello: String
  }
  type Subscription {
    greetings: String
  }
`);
    
const roots = {
  query: {
    hello: () => 'Hello World!',
  },
  subscription: {
    greetings: async function* sayHiIn5Languages() {
      for (const hi of ['Hi', 'Bonjour', 'Hola', 'Ciao', 'Zdravo']) {
        yield { greetings: hi };
      }
    },
  },
};
    
createServer(
  {
    schema,
    execute,
    subscribe,
    roots,
  },
  {
    server, // Your HTTP server
    path: '/graphql',
  }
);

[a-tokyo]

Hey @hongbo-miao

Checkout this apollographql/subscriptions-transport-ws#751 (comment) issue.

The demo you posted won't support a secondary socket, so for example if you want both socket io and gql subs it won't work. That's why there's the extra handling in the demo I posted. To ensure the server can have parallel websockets and not be limited to just 1.

[junminstorage]

Hi All, thanks for the review. I will make the changes to use the new module for subscription. The original implementation I had was posted here in 2018. #436

[junminstorage]

@a-tokyo @hongbo-miao @acao and others, I have worked on this for last few days, the main issue I have with graphql-transport-ws is that it doesn't come with browserfied distribution, while all of our client-side JS requires that, unless we change our build process.

About the concerns of archived dependency is used, please note the main dependency here we used in this PR to support subscription via GraphiQL is subscriptions-transport-ws which is not archived. graphiql-subscriptions-fetcher is very tiny module and I won't concern much about it. Let me know if you insist I can implement and replace it here in this PR.

For the server side, I can update the sample code section in README to use the graphql-transport-ws, which should be easy task as it is clearly documented in its README. However we may lose the consistency cross client and server so developers could be confused.

How about adding another sample code section or point to the alternative module so developers can choose whatever they want?

[enisdenjo]

Hi @junminstorage and others,

author of graphql-transport-ws here 👋 .

I made a conscious decision of not over-engineering the build process for browsers. It boils down to the fact that you will have your own, custom, distribution build process (or you already have one) anyway. Needing support for more browsers is a user facing decision - you will have to adjust the distribution for your app regardless, implicitly adjusting it for the lib too.

Keeping all this in mind, I tend to keep the lib as small and as lean as possible. Less is more in favour of keeping the lib readable, accessible and extendable. Besides, there is not much too polyfill in the lib anyway... It uses pure JS primitives that have been with us for a while now (only exception being the async iterator).

However, if you disagree, want to discuss, ask questions or simply add code - please do open an issue at graphql-transport-ws, I will do my uttermost best to help and steer decisions in favour of the community and a better DX!

Furthermore, the lib can be naturally integrated with express-graphql (taken straight from the lib's readme):

import https from 'https';
import express from 'express';
import { graphqlHTTP } from 'express-graphql';
import { createServer } from 'graphql-transport-ws';
import { execute, subscribe } from 'graphql';
import { schema } from 'my-graphql-schema';

// create express and middleware
const app = express();
app.use('/graphql', graphqlHTTP({ schema }));

// create a http server using express
const server = https.createServer(app);

server.listen(443, () => {
  createServer(
    {
      schema,
      execute,
      subscribe,
    },
    {
      server,
      path: '/graphql', // you can use the same path too, just use the `ws` schema
    },
  );
});

On the side note, I personally wouldn't integrate WebSockets in any GraphQL server. The integration would end up adding more work for the maintainers, forcing the repo to keep up with the libs features and version releases. IMHO, it just feels like superfluous work given the fact of how easy integration actually is... I might be wrong (or off for subscriptions-transport-ws).

[enisdenjo]

Just wanted to touch upon:

either changing our build process here to add browserfy step or add PR to graphql-transport-ws if @enisdenjo is ok with that

I think that the build process should be a user facing decision and liability. I'd honestly like to avoid mixing in the two worlds by supporting various build pipelines. Besides, its also a never ending task - people will want more build options. graphql-transport-ws is about GraphQL over WebSockets, not distribution.

Discussions, opening issues and PRs are always welcome with me! I just like being 100% behind decisions before I go by supporting them.

[acao]

@Urigo i think your subscriptions fetcher would make a lot of sense in the graphiql monorepo, since we were using a clone of your project anyways for examples. we use typescript compiler as the main build toolchain there, and we can add rollup. we got rid of browserify a while ago, since it doesn’t support tree shaking

[Urigo]

couldn't agree more!
The only reason this repo existed was that back then we didn't have the amazing @acao on the project!

[enisdenjo]

Hey guys, just wanted to share that graphql-ws ships UMD builds too as of v1.11.0.

You can now rely on the lib in the browser like this:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8" />
    <title>GraphQL over WebSocket</title>
    <script
      type="text/javascript"
      src="https://unpkg.com/graphql-ws/umd/graphql-ws.min.js"
    ></script>
  </head>
  <body>
    <script type="text/javascript">
      const client = graphqlWs.createClient({
        url: 'wss://umdfor.the/win/graphql',
      });

      // consider other recipes for usage inspiration
    </script>
  </body>
</html>

* Plucked straight out of the lib recipes

[acao]

sounds great, I can use this for the graphiql branch CDN example, too!

[junminstorage]

@acao happy weekend!
Will you be able to provide any direction for us to make progress on this PR? Lots of discussions in this PR are great for future and not directly address to the changes though. Let me know what I miss here.

[acao]

express-graphql is free
to provide whatever fetcher it wants until graphiql has its own reference buildFetcher() library. what y’all have here looks great and i don’t see any reason to block merging. I made progress on a branch for graphiql monorepo to create this new library, but other things came up.

i also don’t think this impacts the IncrementalDelivery implementation in experimental branch, since that doesn’t touch subscriptions afaik, but worth checking into for sure

[junminstorage]

@acao thanks for approving it. I just resolved the merge conflict. Could you or someone else merge it? Let me know what is the next step.

[acao]

would it be considered a breaking change to eventually replace subscriptions-transport-ws with graphql-ws? because we were using that on the branch for the utility function

[junminstorage]

From what I read through the two repos: subscriptions-transport-ws and graphql-ws, the switch should not be breaking changes since the change is internal only.

[enisdenjo]

Awesome to hear that you're considering graphql-ws as a replacement!

Just wanted to give a short disclaimer - beware that the transport Protocol is NOT interchangeable between the two libraries. If you choose one or the other, you must make sure that both the server and the client follow the designated transport protocol.

TL;DR: subscriptions-transport-ws transport Protocol != graphql-ws transport Protocol

[acao]

that's what I had thought. I'm going to ask folks working on the transport spec and the new technical review committee I think. whatever we choose here sets a massive precedent, and we want to be sure to pick the protocol that will make the most sense moving forward. backwards compatible support for servers that implement subscriptions-transport-ws style wss protocol is a factor as well

[enisdenjo]

Using the subprotocol(s) requested by the client in the initial WS handshake, the server can figure out which protocol is necessary and delegate the connection to the relevant lib. So, yeah, you can still support both!

PostGraphile had recently merged a PR doing exactly that: supporting both protocols by delegating connections to the appropriate destination. You can check the PR in question here: graphile/crystal#1338 and the delegator here: https://github.com/graphile/postgraphile/blob/8621b19ff8a0ffd4f245466733e61606967d9c3d/src/postgraphile/http/subscriptions.ts#L460-L485

[acao]

i was just writing a message to @IvanGoncharov suggesting a strategy like this! so, i think this PR is good for now then, since most are still using the original apollo spec. we can implement this logic in a follow up PR or in this PR?

[danielrearden]

@acao @enisdenjo I whole-heartedly disagree with the notion that express-graphql should support both libraries. Denis has single-handedly been driving the WS RFC for the GraphQL over HTTP spec. The spec and its RFCs should be the only thing we worry about supporting and graphql-ws is the only library that would support that goal.

[acao]

it’s very minimal for either when it comes to express-graphql. there is no actual implementation but just some docs entries mostly. the new protocol will be recommended for graphiql, but we should document both. @enisdenjo will provide a follow up PR for graphql-ws

[junminstorage]

Thank you all for the input. I am so glad we moved forward with the subscription support here. I also highly recommend we provided backward compatibility support, for major or big companies like mine, this is very important.

I will continue to provide support for this feature, and @enisdenjo just let me know I can help you create follow-up PR as well.

[enisdenjo]

Hey hey guys, sorry for the wait - my time was absolutely clogged...

graphql-ws now offers a backwards compatibility recipe which can help ease the transitioning phase. Just 2 questions from my side to you, @junminstorage:

• Why have you used the 4 year old version subscriptions-transport-ws@0.5.4 when the latest is v0.9.18? (Maybe the answer is somewhere within these comments, but I didnt bother reading them 😅)
• Would you like to take up the task of adding graphql-ws with backwards compatibility? Shouldn't be a big deal following the new recipe. I just feel you'd be faster having done all this great work already. If not, I'll push a PR when my time frees up - no biggie!

[junminstorage]

@enisdenjo Thanks for the feedback.

1. my original PR was created two years ago: Subscription graphiql support #436. I forgot to upgrade.
2. I have some free time during this holiday season I am happy to work on that.

Just to rephrase our plan here: we will switch to graphql-ws on web client side, then follow your PR at: graphile/crystal#1338 to add delegator on server side to have backwards compatibility support.

[junminstorage]

Per #687 (comment), I have prepared the necessary changes so we can support graphql-ws and subscriptions-transport-ws clients, however I am stuck at one issue may need @enisdenjo 's support, so I opened an issue in his graphq-ws repo. enisdenjo/graphql-ws#93.
Alternatively I can write another subscription fetcher.

[enisdenjo]

I've showcased a simple, minimal, fetcher over at the issue you opened on the lib. Check it out: enisdenjo/graphql-ws#93 (comment).

Hope this helps. If you need further assistance, feel free to ask! I am more than happy to help. 😄