subscription server not properly initialized when taking websocket.Server

[braineo]

What happens

I have an express server running multiple websockets and I'm adding a graphql endpoint with subscription to it.

I found installSubscriptionHandlers took a WebsocketServer but I didn't get any notification, but passing a http server to installSubscriptionHandlers works

I found subscriptions-transport-ws does not think I'm passing a websocket server on this line
https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/server.ts#L132

And actually it received

{
   server: WebSocketServer 
}

I used TypeScript and I try to print my argument out in transpiled js files

// server.js
console.error("Whats my wss2? is it instanceof WebSocketServer?", wss2 instanceof ws_1.default.Server);
aserver.installSubscriptionHandlers(wss2);

Here I got

 Whats my wss2? is it instanceof WebSocketServer? true

// node_modules/apollo-server-core/dist/ApolloServer.js
  installSubscriptionHandlers(server) {
    console.error("let me ask agiain! Whats my wss2? is it instanceof WebSocketServer?", server instanceof ws_1.default.Server);
    console.error("really?????")
    var whatever = new ws_1.default.Server({noServer: true})
    console.error("let me ask this! Whats my whatever? is it instanceof WebSocketServer?", whatever instanceof ws_1.default.Server);

here I got

 let me ask agiain! Whats my wss2? is it instanceof WebSocketServer? false
 really?????
 let me ask this! Whats my whatever? is it instanceof WebSocketServer? true

It does not make sense to me, or is it a common gotcha of instanceof ?

deps

"apollo-server-core": "2.13.1",

import express from "express";
import { ApolloServer, gql } from "apollo-server-express";
import WebSocket from "ws";
import url from "url";
const app = express();
app.set("port", 12345);

const server = app.listen(app.get("port"), () => {});

const typeDefs = gql`

  type Book {
    title: String
    author: String
  }

  type Query {
    books: [Book]
  }
`;

const books = [
  {
    title: "Harry Potter and the Chamber of Secrets",
    author: "J.K. Rowling",
  },
  {
    title: "Jurassic Park",
    author: "Michael Crichton",
  },
];

const resolvers = {
  Query: {
    books: () => books,
  },
};

const aserver = new ApolloServer({ typeDefs, resolvers });

const wss1 = new WebSocket.Server({
  noServer: true,
});

const wss2 = new WebSocket.Server({
  noServer: true,
});

console.error("Whats my wss2? is it instanceof WebSocketServer?", wss2 instanceof WebSocket.Server);
aserver.installSubscriptionHandlers(wss2);

server.on("upgrade", (request, socket, head) => {
  const pathname = url.parse(request.url).pathname;
  if (pathname == "/") {
    wss1.handleUpgrade(request, socket, head, (ws) => {
      wss1.emit("connection", ws, request);
    });
  } else if (pathname == aserver.subscriptionsPath) {
    wss2.handleUpgrade(request, socket, head, (ws) => {
      wss2.emit("connection", ws, request);
    });
  }
});

[abernix]

There was a recent PR, #1966, that aimed to support this. Perhaps you could look at that and see if there is a bug in the implementation?

[braineo]

hi @abernix

I think I'm looking exactly on this change. I made some change here to check if server is a http server.

braineo@ff50055

I would guess because both my application and apollo server has ws dependency

./node_modules/ws
./node_modules/apollo-server-core/node_modules/ws

and due to node_module resolving algorithm, require('ws') is actually requiring different ws?

What do you think?

[braineo]

To example my hypothesis I did experiment in the transpile js file

//node_modules/apollo-server-core/dist/ApolloServer.js
const ws_1 = __importDefault(require("ws"));
const ws_2 = __importDefault(require("../../ws"));
/// ...

installSubscriptionHandlers(server) {
    console.error("Whats my wss2? is it instanceof apollo's WebSocketServer?", server instanceof ws_1.default.Server);
    console.error("Whats my wss2? is it instanceof my's WebSocketServer??", server instanceof ws_2.default.Server)

And I got following prints

Whats my wss2? is it instanceof apollo's WebSocketServer? false
Whats my wss2? is it instanceof my's WebSocketServer?? true

[abernix]

@jedwards1211 Thoughts on this and the PR?

[jedwards1211]

Yeah sounds like a duplicate module issue. Maybe we could use some check besides instanceof to determine if the passed thing looks like a websocket server.

[jedwards1211]

Braineo's solution is pretty smart, since native modules (http) wouldn't suffer instanceof issues

Edit: probably should check for https too though? I forget if the class is different for https

[jedwards1211]

The only other thing I would suggest is to support passing {webSocketServer: ...} to explicitly indicate what we want, but probably not necessary

[timkock]

@braineo I just ran into the same problem where I was using socket.io via express as well as the GraphQL subscription socket. After reconfiguring express with the following hint, I was able to reproduce your experiment by the following two changes:

1. Adding this line in the requirements up top
   const ws_2 = __importDefault(require("../../ws"));

2. And changing the server instanceof ws_1.default.Server to server instanceof ws_2.default.Server

Was all that was required to make it work and I finally started seeing subscription messages coming through again. The instanceof doesn't work as you can't guarantee it coming from the same module.

What would be a good simple robust change for a pull request, reading through the ROADMAP.md I think the whole transport will be re-written, is being re-written so will just monkey patch for now...

[braineo]

@timkock I doing monkey patch too. Waiting for maintainers to take a look at this one #4200

[timkock]

@braineo & others, the patch that is in the other issue is typescript, the current repo is mostly just Javascript so here is a gist with a working patch in just Javascript https://gist.github.com/timkock/b38ae86cf5634c63dc482c7fc1c66be1