New GraphQLWsLink for graphql-ws subscriptions library #9369

glasser · 2022-01-28T20:35:32Z

Apollo Client currently contains WebSocketLink in
@apollo/client/link/ws which uses the subscriptions-transport-ws
library. That library is no longer actively maintained, and there is an
improved fork called graphql-ws.

The two libraries use different protocols so a different client link is
required for graphql-ws. (While the WebSocket protocol does allow for
subprotocol negotiation, neither server implementation supports this in
a practical way.)

This PR adds a new link GraphQLWsLink in
@apollo/client/link/subscriptions. Its constructor arguments are the
same as the createClient function in graphql-ws (or it can take a
Client object returned from that function), and you need to install
the optional peer dep graphql-ws instead of
subscriptions-transport-ws. Once you've created the link, it works
exactly like the old WebSocketLink.

This PR changes the main subscriptions doc page to mostly document the
new link, with an extra section at the bottom for the old link.

The core GraphQLWsLink code is based on MIT-licensed code from the
README of the graphql-ws repository.

Fixes #8345.
Part of apollographql/apollo-server#6058

docs/source/api/link/apollo-link-subscriptions.md

glasser · 2022-01-28T22:12:28Z

(fixed accidental prettier applications)

brainkim

Thanks for doing this @glasser. Some thoughts, but nothing blocking.

src/link/subscriptions/index.ts

src/link/subscriptions/__tests__/graphqlWsLink.ts

brainkim · 2022-01-31T16:20:33Z

src/link/subscriptions/index.ts

+export class GraphQLWsLink extends ApolloLink {
+  private client: Client;
+
+  constructor(optionsOrClient: ClientOptions | Client) {


There is very little benefit to this sort of polymorphism, except maybe consistency and users not having to import graphql-ws directly. However, I think the latter pro can also be considered a a con, as people are likely to improperly add graphql-ws to their dependencies, or improperly configure the client and then open up an issue with us. Also, the type-sniffing of a dependency creeps me out because this would break if graphql-ws ever added a subscribe property directly to the client object. Therefore, I think it might be better to BYOWSC (bring your own websocket client).

But it’s not that big a deal.

That's my general philosophy as well. I was echoing the existing link here, but very happy to just require you to use graphql-ws directly. It simplifies the doc too. @benjamn Any thoughts or shall I make this change?

Since Client is just an interface in the graphql-ws library, I agree it's important to preserve the ability to provide your own Client, and we should not assume createClient will always be the source of the implementation. In other words, I agree with @brainkim that the constructor should take a Client and not ClientOptions (so it doesn't have to do any shape sniffing).

I'm sympathetic to the argument that new GraphQLWsLink({ url }) seems more ergonomic in the common case than new GraphQLWsLink(createClient({ url })), but the first version forces us to call (and bundle!) createClient (a rather large function—333 lines) behind the scenes, much like new ApolloClient({ resolvers }) forces us to use/bundle our old LocalState implementation, whereas something more verbose like

new ApolloClient({ localState: new LocalState({ resolvers }), })

would allow developers to completely omit our LocalState implementation, or provide their own. In other words, the simplicity of new GraphQLWsLink({ url }) has some large hidden costs.

With all that said, I believe we could safely re-export createClient from this module, without impacting bundle sizes when it's not used, just so folks don't have to import it from graphql-ws directly if they don't want to.

OK, I think I want to move towards just taking the client (not creation options) and letting you import it yourself from graphql-ws rather than re-exporting it. I don't think importing a function from a package you're required to install yourself anyway (optional peer dep) is that big a deal.

At this point, the only non-test dependency on graphql-ws will be for the Client type (which is an exposed part of the GraphQLWsLink API). Should we still have this as an optional peer dep? Or is there a simpler alternative? It would be nice if there was a mechanism in npm/ts for "type-only dep" but I don't think there really is one (devDependencies aren't appropriate for types that are actually used in the depending package's API, I think). I guess we could also just copy the Client interface into the file or something but that's not great either.

I think the optional peer dep is still the way to go. Better to reuse the original Client type than copy it over.

benjamn

Looking good, with a few questions/comments/responses. Thanks especially for all this documentation @glasser!

docs/source/api/link/apollo-link-ws.md

docs/source/data/subscriptions.mdx

src/link/subscriptions/__tests__/graphqlWsLink.ts

benjamn · 2022-02-01T21:42:40Z

src/link/subscriptions/index.ts

+export class GraphQLWsLink extends ApolloLink {
+  private client: Client;
+
+  constructor(optionsOrClient: ClientOptions | Client) {


Since Client is just an interface in the graphql-ws library, I agree it's important to preserve the ability to provide your own Client, and we should not assume createClient will always be the source of the implementation. In other words, I agree with @brainkim that the constructor should take a Client and not ClientOptions (so it doesn't have to do any shape sniffing).

I'm sympathetic to the argument that new GraphQLWsLink({ url }) seems more ergonomic in the common case than new GraphQLWsLink(createClient({ url })), but the first version forces us to call (and bundle!) createClient (a rather large function—333 lines) behind the scenes, much like new ApolloClient({ resolvers }) forces us to use/bundle our old LocalState implementation, whereas something more verbose like

new ApolloClient({ localState: new LocalState({ resolvers }), })

would allow developers to completely omit our LocalState implementation, or provide their own. In other words, the simplicity of new GraphQLWsLink({ url }) has some large hidden costs.

With all that said, I believe we could safely re-export createClient from this module, without impacting bundle sizes when it's not used, just so folks don't have to import it from graphql-ws directly if they don't want to.

src/link/subscriptions/index.ts

glasser · 2022-02-01T23:06:39Z

@brainkim @benjamn I believe all feedback has been addressed.

Apollo Client currently contains `WebSocketLink` in `@apollo/client/link/ws` which uses the `subscriptions-transport-ws` library. That library is no longer actively maintained, and there is an improved fork called `graphql-ws`. The two libraries use different protocols so a different client link is required for `graphql-ws`. (While the WebSocket protocol does allow for subprotocol negotiation, neither server implementation supports this in a practical way.) This PR adds a new link `GraphQLWsLink` in `@apollo/client/link/subscriptions`. Its constructor arguments are the same as the `createClient` function in `graphql-ws` (or it can take a `Client` object returned from that function), and you need to install the optional peer dep `graphql-ws` instead of `subscriptions-transport-ws`. Once you've created the link, it works exactly like the old `WebSocketLink`. This PR changes the main subscriptions doc page to mostly document the new link, with an extra section at the bottom for the old link. The core GraphQLWsLink code is based on MIT-licensed code from the README of the graphql-ws repository. Fixes #8345. Part of apollographql/apollo-server#6058

Co-authored-by: Ben Newman <[email protected]>

glasser · 2022-02-01T23:34:04Z

@benjamn Rebased and retargeted to release-3.6, with CHANGELOG added.

brainkim

Thank you for bearing with my nits!

darkbasic · 2022-02-02T21:11:26Z

Would it be possible to release a new beta with this?

New GraphQLWsLink for graphql-ws subscriptions library

New `GraphQLWsLink` and `@apollo/client/link/subscriptions` sub-package entry point for `graphql-ws` subscriptions library.

glasser force-pushed the glasser/graphql-ws branch from 5349c38 to 7c6bc13 Compare January 28, 2022 21:13

glasser changed the title ~~[wip for graphql-ws support]~~ New GraphQLWsLink for graphql-ws subscriptions library Jan 28, 2022

glasser requested review from benjamn and brainkim January 28, 2022 21:14

glasser marked this pull request as ready for review January 28, 2022 21:14

glasser requested a review from StephenBarlow as a code owner January 28, 2022 21:14

lennyburdette reviewed Jan 28, 2022

View reviewed changes

docs/source/api/link/apollo-link-subscriptions.md Outdated Show resolved Hide resolved

glasser force-pushed the glasser/graphql-ws branch 2 times, most recently from 76dc22a to 3f32357 Compare January 28, 2022 22:10

brainkim reviewed Jan 31, 2022

View reviewed changes

benjamn reviewed Feb 1, 2022

View reviewed changes

glasser force-pushed the glasser/graphql-ws branch from 85a3327 to 464c9c6 Compare February 1, 2022 22:55

benjamn approved these changes Feb 1, 2022

View reviewed changes

glasser and others added 9 commits February 1, 2022 15:31

Update docs/source/api/link/apollo-link-ws.md

a8ff094

Co-authored-by: Ben Newman <[email protected]>

Update docs/source/data/subscriptions.mdx

a573c8e

Co-authored-by: Ben Newman <[email protected]>

rename sink -> observer

a10f48d

Use ApolloError

a1fcb6c

Don't accept createClient options; just accept a Client

b453ced

Document old link preferring explicit SubscriptionClient

a3604e4

Adopt isLikeCloseEvent from graphql-ws

b344f00

Use AC isNonNullObject instead of graphql-ws copy-paste version

11c0051

glasser force-pushed the glasser/graphql-ws branch from b597925 to 11c0051 Compare February 1, 2022 23:32

glasser changed the base branch from main to release-3.6 February 1, 2022 23:32

Add CHANGELOG

4caf088

brainkim approved these changes Feb 1, 2022

View reviewed changes

benjamn merged commit d98f1de into release-3.6 Feb 2, 2022

benjamn deleted the glasser/graphql-ws branch February 2, 2022 16:22

benjamn restored the glasser/graphql-ws branch February 2, 2022 16:22

glasser mentioned this pull request Feb 15, 2022

Fully archive subscriptions-transport-ws repository apollographql/apollo-server#6058

Closed

7 tasks

benjamn added a commit that referenced this pull request Feb 24, 2022

Merge pull request #9369 from apollographql/glasser/graphql-ws

6ef4fa4

New GraphQLWsLink for graphql-ws subscriptions library

benjamn mentioned this pull request Feb 24, 2022

Backport PR #9369 to main branch for GraphQLWsLink and @apollo/client/link/subscriptions #9453

Merged

benjamn added a commit that referenced this pull request Feb 24, 2022

Backport PR #9369 to main branch for GraphQLWsLink (#9453)

ce90c47

New `GraphQLWsLink` and `@apollo/client/link/subscriptions` sub-package entry point for `graphql-ws` subscriptions library.

SirMishaa mentioned this pull request Mar 4, 2022

Provide a new modern pusher link implementation for Subscription for Apollo Client (based on Graphql-ws) nuwave/lighthouse#2079

Open

benjamn mentioned this pull request Mar 30, 2022

[docs] Repair import file path for GraphQLWsLink #9566

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New GraphQLWsLink for graphql-ws subscriptions library #9369

New GraphQLWsLink for graphql-ws subscriptions library #9369

glasser commented Jan 28, 2022 •

edited

Loading

glasser commented Jan 28, 2022

brainkim left a comment

brainkim Jan 31, 2022

glasser Jan 31, 2022

benjamn Feb 1, 2022 •

edited

Loading

glasser Feb 1, 2022

glasser Feb 1, 2022

benjamn Feb 1, 2022

benjamn left a comment

benjamn Feb 1, 2022 •

edited

Loading

glasser commented Feb 1, 2022

glasser commented Feb 1, 2022

brainkim left a comment

darkbasic commented Feb 2, 2022

New GraphQLWsLink for graphql-ws subscriptions library #9369

New GraphQLWsLink for graphql-ws subscriptions library #9369

Conversation

glasser commented Jan 28, 2022 • edited Loading

glasser commented Jan 28, 2022

brainkim left a comment

Choose a reason for hiding this comment

brainkim Jan 31, 2022

Choose a reason for hiding this comment

glasser Jan 31, 2022

Choose a reason for hiding this comment

benjamn Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

glasser Feb 1, 2022

Choose a reason for hiding this comment

glasser Feb 1, 2022

Choose a reason for hiding this comment

benjamn Feb 1, 2022

Choose a reason for hiding this comment

benjamn left a comment

Choose a reason for hiding this comment

benjamn Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

glasser commented Feb 1, 2022

glasser commented Feb 1, 2022

brainkim left a comment

Choose a reason for hiding this comment

darkbasic commented Feb 2, 2022

glasser commented Jan 28, 2022 •

edited

Loading

benjamn Feb 1, 2022 •

edited

Loading

benjamn Feb 1, 2022 •

edited

Loading