Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Support graphql-ws client for GraphiQL IDE #739

Closed
wants to merge 7 commits into from

Conversation

junminstorage
Copy link
Contributor

@junminstorage junminstorage commented Jan 25, 2021

To follow up #687 and continue to address #390, I drafted this PR to

Questions:
1. There is no umd version of graphql parser, e.g. graphql-tag publish a umd parser but it has dependency on graphql itself. I use a simple regex to check if the graphQLParams contains subscription operation. Options:
- wait for graphql/graphiql#1770 to finish so we can use its umd fetcher which supports both ws clients
- create umd fetcher or graphql parser in this repo.

2. developer provides websocketClient as part of graphiql option, its values are "v0" for subscriptions-transport-ws, "v1" for graphql-ws, I did this based on graphile/crystal#1338, open to better naming suggestion.

Specifically check out this tutorial script about how to start web socket server to support subscription using enisdenjo/graphql-ws : https://github.com/junminstorage/graphql-tutorial/blob/master/src/index_v1.js

@acao
Copy link
Member

acao commented Jan 26, 2021

hey this is awesome!

just wanted to point out that this is in the works as well:
graphql/graphiql#1770

we were hoping to publish this library alongside GraphiQL, and then here we can use that utility to generate the fetcher as well. what do you think?

@junminstorage
Copy link
Contributor Author

That will be perfect! Note the apollographql/GraphiQL-Subscriptions-Fetcher is already archived and doesn't work for https://github.com/enisdenjo/graphql-ws, so to have a generic and up-to-dated fetcher will be great!

@acao
Copy link
Member

acao commented Jan 26, 2021

@junminstorage to be honest, it should never have ended up in this repo. i had asked someone to use the subsriptions fetcher as prior art to build something new and they installed the package instead, haha! oops!

@junminstorage
Copy link
Contributor Author

@acao I looked at your great work on @graphiql/create-fetcher in graphql/graphiql#1770, it seems you are very close to finish line. It is big PR, do you have any ETA? I wonder if it is worth creating a dedicated PR for fetcher related changes.

@acao
Copy link
Member

acao commented Jan 29, 2021

@junminstorage hey thank you! i hope to finish it by tomorrow

@acao
Copy link
Member

acao commented Feb 6, 2021

@junminstorage forgot to update you, you should be good to pull in @graphiql/create-fetcher and get that set up!

Base automatically changed from master to main February 10, 2021 15:09
@junminstorage
Copy link
Contributor Author

@achao I run into issue with the @graphiql/create-fetcher, see this issue: graphql/graphiql#1800 please help.

Also another question, you provide https://github.com/graphql/graphiql/blob/main/packages/graphiql-toolkit/docs/create-fetcher.md#wsclient option for developer to inject graphql-ws client dependency, could you provide another option for developer to inject subscriptions-transport-ws as well. This allows graphiql/toolkit not tied to any particular versions of both clients.

@acao
Copy link
Member

acao commented Feb 11, 2021

it was a matter of significant debate to even get subscriptions-transport-ws into this utility, so we may need to revisit that since graphql-ws is being worked into the official transport spec, whereas subscriptions-transport-ws is abandoned and has a deprecation notice.

we are about to publish it under @graphiql/toolkit as an RC, since we don't need a seperate package, and then release 0.1.0, so it will be import { createFetcher } from '@graphiql/toolkit' next release

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 12, 2021

CLA Missing ID

  • ❌ The commit (6ae753b ,89d1ba917bc7caf119f570766b3a7c8639f41219 ,f19a7bcf73d057ea80a06cb10ff04eaa0ef3ba96 ,7bb97ce16be109d4c8c891a7e5a8566d72d3c960 ,19ecf24d551abf7fbd575b3b413fc7a2edf338bb ,676a6eb5ee226362ed67b10a9bf4bbe8c92e36d2) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

@junminstorage
Copy link
Contributor Author

@acao all changes are ready!
But I missed up my commit id in this PR draft and will recreate new branch and new PR with correct username.

@junminstorage
Copy link
Contributor Author

replaced by #755

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants