GraphiQL.createFetcher does not work for subscriptions-transport-ws

[junminstorage]

As shown in this PR on express-graphql: https://github.com/graphql/express-graphql/pull/739/files#diff-f851f2ce2fbcb0220aaca183def93d7ced5b533fead2016551cfd6d37b357ae5, I started the graphql server with subscription using legacy subscriptions-transport-ws.

Then on the client side I create fetcher like this:

window.GraphiQL.createFetcher({url: fetchURL, subscriptionUrl: ${safeSerialize(
            subscriptionEndpoint,
          )}});

Then I got error as shown in the below screenshot:

The same client side code works if I started a graphql server using graphql-ws though:
https://github.com/graphql/express-graphql/pull/739/files#diff-4f5f57981087dfef3de9e960e86656c7a1d1ef9f424fd0a75e32751163a286e1

[acao]

@junminstorage thank you for looking into this, this is strange and not how it should behave.

[junminstorage]

I also pulled in @graphiql/toolkit since the fetcher is moved under toolkit now, I still got the same error. The bug may be somewhere in those lines in this function?

graphiql/packages/graphiql-toolkit/src/lib.ts

Lines 70 to 80 in 3286b9c

	export const createWebsocketsFetcherFromUrl = (
	url: string,
	connectionParams?: ClientOptions['connectionParams'],
	) => {
	let wsClient: Client | null = null;
	let legacyClient: SubscriptionClient | null = null;
	if (url) {
	try {
	try {
	// TODO: defaults?
	wsClient = createClient({

[acao]

@junminstorage no changes have been made to address your bug yet. we will probably remove support for the legacy protocol

[acao]

I will let you know when there is an update, but for now just letting you know that the location of the package has changed

[junminstorage]

I tried out the approach as in this function:

graphiql/packages/graphiql-toolkit/src/lib.ts

Line 70 in 3286b9c

export const createWebsocketsFetcherFromUrl = (

, both wsClient and legacyClient can be initialized against either of two types of subscription ws servers without any errors being thrown. so it indeed is a bug.

the difference is that, if we add a listener for connected event using legacy client, like below:

          let client = new SubscriptionClient(${safeSerialize(subscriptionEndpoint)}, {
            reconnect: true
          });
         client.on("connected", listenerFn);

The listenerFn function will only be called if it is subscription-transport-ws server.

The express-graphql is already on legacy ws client, it will be great if we support it. how hard is to add legacyClient as one of the options in createFetcher?

[acao]

express graphql should not be using deprecated packages imo. the legacy protocol is deprecated, so we will remove it in the next release

[junminstorage]

https://www.npmjs.com/package/subscriptions-transport-ws still have 1.7 millions weekly download. we have the chance to support both protocols with little cost here.

if we remove it from express-graphql, next release will be breaking change. And we also need to make change and fix the bug in this rc as well.

anyway, just let me know the direction we eventually take, I can help with this issue as well.

[acao]

@junminstorage do you think you could introduce a PR for this fix then? maybe collaborate with @ryan-m-walker and/or @imolorhe since i’m taking a break?

[junminstorage]

Surely and happy to do so. Let me draft a PR to start the discussion with them.

[acao]

I have a fix coming!

[acao]

@junminstorage introduced the legacyClient option as requested in the above PR, and just made it so that only graphql-ws is tried with subscriptionsUrl since the failover logic is too complex and unnecessary, when most projects are instantiating a custom client instance anyways.

thanks for all your work @junminstorage getting this working in the ecosystem, hoping to help you get everything working nicely here and in express-graphql in the following weeks!

[junminstorage]

@acao make sense. thank you!