feat(subscriptions): enable passing WebSocket.Server instead of HttpServer.

[jedwards1211]

For people who need to have multiple WebSocket servers sharing a single HTTP(S) server, the ws docs recommend creating multiple instances of new WebSocket.Server({ noServer: true }). This PR makes installSubscriptionHandlers accept a WebSocket.Server instance (in addition to accepting an http server) to support the multiple WebSocket servers use case.

Real-life example from my code:

      const graphQLWebSocket = new WebSocket.Server({
        noServer: true,
      })
      apolloServer.installSubscriptionHandlers(graphQLWebSocket)

      const symmetryWebSocket = new WebSocket.Server({
        noServer: true,
      })
      symmetryWebSocket.on('connection', socket => new SymmetryServer({socket}))

      httpServer.on('upgrade', (request: IncomingMessage, socket: net$Socket, head: any) => {
        if (request.url.startsWith(SYMMETRY_BROWSER_PATH)) {
          symmetryWebSocket.handleUpgrade(request, socket, head, (ws: WebSocket) => {
            symmetryWebSocket.emit('connection', ws)
          })
        } else if (request.url.startsWith(GRAPHQL_PATH)) {
          graphQLWebSocket.handleUpgrade(request, socket, head, (ws: WebSocket) => {
            graphQLWebSocket.emit('connection', ws)
          })
        } else {
          socket.destroy()
        }
      })

TODO:

• Update CHANGELOG.md with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests (NOTE: installSubscriptionHandlers is not currently tested anywhere)
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[jedwards1211]

@abernix can we get this merged?

[jedwards1211]

I fixed the conflict in the changelog, but I hope you can merge this soon so I don't have to keep merging conflicts over and over again

[LmKupke]

@abernix this would be great to have

[jedwards1211]

@abernix please include this in v3

[vincentdesmares]

@abernix please include this in v3

[vincentdesmares]

I strongly support this pull request as I find very difficult to patch it on a fork. Sadly we use apollo-server-express, which depends on apollo-server-core. So I would have to fork multiple projects in order to use this patch :(

@martijnwalraven @trevor-scheer When do you plan the next release of apollo-server-core?

[jedwards1211]

@vincentdesmares here's my current workaround

[vincentdesmares]

Thank you so much @jedwards1211 !!!

I did not exactly use your solution as my server is instantiated by a sub-library. But it gave me the idea of monkey-patching the already instantiated Object!

apiServer.installSubscriptionHandlers = yourInstallSubscriptionHandlers
jobsServer.installSubscriptionHandlers = yourInstallSubscriptionHandlers

And it works perfectly!

That will do it until they release the patch :)

[jedwards1211]

You're welcome! Yeah what I did with Object.create is probably unnecessarily complicated.

[jak-pan]

Please merge this, +1 needed here.

[OmgImAlexis]

@jedwards1211 how is the work around meant to be used?

Currently have something like this, cut down for example sake.

import stoppable from 'stoppable';
import express from 'express';
import http from 'http';
import { ApolloServer } from 'apollo-server-express';
import { graphql } from './graphql';

const app = express();

const graphApp = new ApolloServer(graphql);
graphApp.applyMiddleware({app});

const httpServer = http.createServer(app);
const stoppableServer = stoppable(httpServer);
graphApp.installSubscriptionHandlers(stoppableServer);

[jedwards1211]

@OmgImAlexis basically just monkeypatch the installSubscriptionHandlers function, and then see my OP for how to set up multiple websocket handlers with it.

It's not clear from your example that you need this -- this workaround/PR is intended for cases where you need multiple separate websocket handlers. Do you need multiple websocket handlers?

[jedwards1211]

@martijnwalraven @abernix will this PR ever get merged?

Will we ever get to stop monkeypatching?

[OmgImAlexis]

@jedwards1211 sorry to be a bother but how exactly do I do that?

[jedwards1211]

import express from 'express'
import http from 'http'
import WebSocket from 'ws'
import { ApolloServer } from 'apollo-server-express'

const app = express()
const httpServer = http.createServer(app)
const graphQLWebSocket = new WebSocket.Server({ noServer: true })
const otherWebSocket = new WebSocket.Server({ noServer: true })

const GRAPHQL_PATH = '/graphql'
const OTHER_WEB_SOCKET_PATH = '/other'

httpServer.on('upgrade', (request, socket, head) => {
  const { url } = request
  if (url.startsWith(GRAPHQL_PATH)) {
    graphQLWebSocket.handleUpgrade(request, socket, head,ws => {
      graphQLWebSocket.emit('connection', ws)
    })
  } else if (url.startsWith(OTHER_WEB_SOCKET_PATH)) {
    otherWebsocket.handleUpgrade(request, socket, head, ws => {
      otherWebsocket.emit('connection', ws)
    })
  } else {
    socket.destroy()
  }
})

const apolloServer = new ApolloServer({
  path: GRAPHQL_PATH,
  // you'll need to provide/flesh out the following
  schema,
  context: ({req, connection}) => ...,
  subscriptions: {
    path: GRAPHQL_PATH,
    onConnect: (connectionParams, websocket) => ...,
  },
})
apolloServer.installSubscriptionHandlers = <the big function from my gist>
apolloServer.applyMiddleware({ app, path: GRAPHQL_PATH })
apolloServer.installSubscriptionHandlers(graphQLWebSocket)

otherWebSocket.on('connection', ...)

httpServer.listen(4000)

[veeramarni]

This PR is opened for a while and the solution seems to be elegant to have own websocket configuration. @abernix @martijnwalraven can this PR be reviewed quickly and merge it?

[kulakowka]

I use multiple graphql schemes on a single http server. I would like to be able to run several web socket servers. I would like this PR to be merged.

[abernix]

This looks non-controversial enough to bring in without being a breaking change. Sorry this has sat for so long! We're trying to make our way through the backlog of things to be addressed, but subscriptions has often taken a backseat. We're working to consider it more carefully going forward, but do bear with us while we work toward eventually making it a more first-class citizen (i.e. actually benefit from the request pipeline and what it brings in terms of plugin support, caching, observability, etc.)

[jedwards1211]

@abernix thanks a bunch!

Even https://graphql.org/learn/ still doesn't even mention subscriptions AFAIK, so subs are kind of the second-class citizen of the whole ecosystem.

Food for thought: in one of my projects I've started a pattern of using matching query and subscription fields implement an "observable" queries. For it to work I have to do a cache-only query, a no-cache subscription, and do all the cache updates in onSubscriptionData. The backend sends the initial result set as the first subscription event.

[okeren-cx]

I've run into the same problem - needing to run two WS on the same http server, but for some reason I can't get the subscriptions to work. Work is based off of the examples for ws and the snippets shown here:

const httpServer = createServer(app);
const timeSyncWebSocketServer = new WebSocket.Server({ noServer: true });
const timesync = new TimeSync(timeSyncWebSocketServer);

const graphQLWebSocketServer = new WebSocket.Server({ noServer: true });
graphqlServer.installSubscriptionHandlers(graphQLWebSocketServer);


httpServer.on('upgrade', (request: IncomingMessage, socket, head: any) => {
  const pathname = url.parse(request.url).pathname;

  if (pathname === '/timesync') {
    timeSyncWebSocketServer.handleUpgrade(request, socket, head, (ws: WebSocket) => {
      timeSyncWebSocketServer.emit('connection', ws);
    });
  } else if (pathname === '/graphql') {
    graphQLWebSocketServer.handleUpgrade(request, socket, head, (ws: WebSocket) => {
      graphQLWebSocketServer.emit('connection', ws);
    });
  } else {
    socket.destroy();
  }
});


timesync.listen();
const tournamentsServer = httpServer.listen(app.get('port'), () => {

The timesync feature works great, it enters the .on('connection... block properly in that class, but for some reason the graphql subscriptions don't work at all. The block for the /graphql pathname is executed, so it seems that maybe the emit isn't getting picked up? not sure really, there's no error either.
We're using apollo-server-express v2.14.4.

With a single websocket the subscriptions work great:

const httpServer = createServer(app);
graphqlServer.installSubscriptionHandlers(httpServer);
const server = httpServer.listen(app.get('port'), () => {

but once we add another websocket it doesn't. Does anyone have any idea what might be going on or how to debug this?

[jedwards1211]

@okeren-cx you might be having issue #4198, check if there are multiple versions of ws in your node_modules.

[okeren-cx]

@jedwards1211 thanks! it was indeed the problem. I ended up monkey patching in the same way you showed here and it worked.

[timkock]

@jedwards1211 and others I had this issue and the issue in #4198.

TL;DR until some changes are merged or transport is changed the monkey patch is the way to make it work, this merge request doesn't solve #4198. The version by @jedwards1211 is in typescript, here is a working javascript equivalent. Thanks to all people that did a lot of the digging before 👍

https://gist.github.com/timkock/b38ae86cf5634c63dc482c7fc1c66be1