allow using custom WebSocket server implementation

[lgandecki]

Hello, this will allow me to write isolated integration tests for subscriptions.
My next step will be to create apollo-link-schema-with-subscriptions (open to a better name! :) ), so people can simply do:

new ApolloClient({
    link: new SchemaLinkWithSubscriptions({
      schema,
      context
    }),
    cache: new InMemoryCache()
  });

And be able to use apolloClient as if it was connected to a websocket server (so they can do subscriptions as well)

Currently, with my fork of subscriptions-transport-ws in my test I do:

const mockServer = require("mock-socket-with-protocol").Server;
const mockWebSocket = require("mock-socket-with-protocol").WebSocket;
const gqClient = () => {
  const port = Math.random(); 
// using incorrect port here - it doesn't matter 
// since the mocked implementation does string matching to connect client/server anyway,
// there are no actual ports being opened, which is the point ;-) 
  SubscriptionServer.create(
    {
      schema,
      execute,
      subscribe
    },
    { host: "ws://localhost", port },
    mockServer
  );
  
  const wsLink = new WebSocketLink({
    uri: `ws://localhost:${port}`,
    webSocketImpl: mockWebSocket
  });

  return new ApolloClient({
    link: wsLink,
    cache: new InMemoryCache()
  });
};

and it works like a charm :)

• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change

[mxstbr]

I definitely see the value in being able to pass a custom websocket server! 💯

The API is kinda awkward, as you end up having to pass socketOptions even though it's not used since you also pass your own server:

new SubscriptionServer({}, {}, customServer);

What about instead letting the second argument be either socketOptions or a custom server?

new SubscriptionServer(options: ServerOptions, socket: WebSocket.ServerOptions | WebSocket)

That seems much nicer from the user side: either just pass options and it'll work, or if you need more control you can also pass your own server instead(!).

[lgandecki]

Hey Max! You are right. Let me tweak this tomorrow morning :)
Happy to hear you are not opposed to the idea - meaning I (most possibly!) won't have to use a fork.

[mxstbr]

Awesome, much better! Now it'd be great if you could add a note about this to the docs and the readme and we should be good to go.

[lgandecki]

@mxstbr I added some docs and examples - please let me know if this is a good direction. I can continue on this tomorrow. :) Thanks!

[mxstbr]

The "works with custom WebSocket implementation" test fails on that latest commit, any clues why?

[mxstbr]

Readme doc update is 💯, but let's work on the website a bit more.

Currently it's a bit too personal, and also the example code shouldn't be everything that's needed ever, it should only demonstrate the core concepts. (creating a custom websocket server with mock-socket-with-protocol, passing that to the subscription server and writing the test) Mind revising that?

[lgandecki]

As per the failure.. looks like it's a rare timing issue, looks like 200 ms between firing the events and the assertion that the subscription data came back wasn't enough. I only updated Changelog so that couldn't affect the test. I can increase the timeout a bit. A nicer way of doing this might be to instead of having an assertion in a timeout to check that both subscriptions finished:

if (result.data) {
    numTriggers += 1;
    assert.property(result.data, 'userFiltered'); (..)
    if(numTriggers === 2) {
        done()
    }
}

in both of them. This way the test would end the moment both subscriptions came back, instead of waiting a particular amount of time to verify that numOfTriggers is equal 2. I reused the pattern that's been established in the previous tests, but I don't think it's ideal to be honest.

Website part - would you rather it not speak about apollo and have a test that's almost a copy paste of the one that I talked about above?
Or maybe not even have a page about integration testing, but an example of creating a server and connecting a client to it?

I thought that the most typical use case would be nice to cover :) but I could change that into a blog post, and have more minimal info here.

Thanks!

[mxstbr]

This is ~what I'd like to see in terms of the example in the docs:

// we are not opening any ports - this example of WebSocket client/server uses string matching to know to what server connect a given client
// we should use different string for every test to not share state
  const RANDOM_WS_PORT = Math.floor(Math.random() * 100000);

  const customServer = new Server(`ws://localhost:${RANDOM_WS_PORT}`);

  // We pass customServer instead of typical configuration of a default WebSocket server
  SubscriptionServer.create(
    {
      schema,
      execute,
      subscribe
    },
    customServer
  );

  const wsLink = new WebSocketLink({
    uri: `ws://localhost:${RANDOM_WS_PORT}`,
    webSocketImpl: WebSocket
  });

  return new ApolloClient({
    link: wsLink,
    cache: new InMemoryCache()
  });
};

test(
  "subscription",
  async done => {
    gqClient()
      .subscribe({
        query: gql`
          subscription {
            somethingChanged
          }
        `
      })
      .subscribe({
        next({ data }) {
          expect(data.somethingChanged).toEqual("passedId");
          done();
        },
        error(err) {
          throw new Error(err);
        }
      });

    setTimeout(() => {
      pubsub.publish(SOMETHING_CHANGED_TOPIC, "passedId");
    }, 100);
  },
  1000
);

Maybe?

The text should specify when to even use that code in the first place, rather than the "I" stuff.

[lgandecki]

I see, thanks for the feedback. I will take a look tomorrow. I get your point. I was torn here, obviously there is a lot of boiler code that's not related to the new functionality and it introduces a lot of noise.. On the other hand this is a piece of code that you can copy/paste and it works (assuming you install all the dependencies, but that's the easy part). Otherwise it might be a bit tricky to understand what all the parts are and how they connect with each other.

[lgandecki]

I finally got some time to clean it up a bit. I hope that's better.

On a different note - I'm also tempted to work on the tests, they are slow, and can't be parallelized. In mocha watch mode they take ~25 seconds to run, which is a rather suboptimal developer experience. :-)

If it was up to me I'd:

• split client/server to separate files (so they can run in parallel)
• introduce jest for its fantastic watch mode
• use waitForExpect instead of arbitrary setTimeouts that introduce unnecessary long waits in some places.

Is this something you guys would consider merging?

[NeoPhi]

I'd be happy moving to jest as the testing framework.

[mxstbr]

Same, I'd love to actually, but please do it in a separate PR!

[mxstbr]

This LGTM, @NeoPhi?

[NeoPhi]

@mxstbr Yes, looks good.

[lgandecki]

Ping :-)

[lgandecki]

I have people asking me about this, even on apollo slack - is there any reason why this is not merged yet? Is there a different pattern for doing integration testing for subscriptions?
I personally use my fork, but would definitely prefer to just be able to install and use the main package..

[NeoPhi]

@lgandecki Fell off the radar. Merging and releasing now.

[danfernand]

Question please, how do you pass in the constant to publish for in

pubsub.publish(SOMETHING_CHANGED_TOPIC, "passedId");

example subsscription server

SubscriptionServer.create(
    {
      schema: gql`
        type SomethingLog {
          log: String
        }

        type Subscription {
          onSomethingLog(
            someId: ID!
          ): SomethingLog
        }
      `,
      execute,
      subscribe
    },
    customServer
  );

Im trying to trigger a publish via pubsub but I am not piecing it all together