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

feat: support graphql-ws client for GraphiQL IDE #755

Merged
merged 7 commits into from
Jun 1, 2021

Conversation

junminstorage
Copy link
Contributor

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

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

You can also checkout the two newly added examples inside /examples directory in this PR for usage.

@junminstorage
Copy link
Contributor Author

@acao this is the newly created PR. it looks all recently opened PRs failed on tests running on Node v12. Do you know why?

@acao
Copy link
Member

acao commented Apr 13, 2021

@junminstorage not sure, appears to be the same issue on a few PRs and I can’t seem to find the actual error! Looking into it tonight, we will have this and all the subscriptions support docs released by friday at latest

@junminstorage
Copy link
Contributor Author

junminstorage commented Apr 13, 2021

@acao I think it is something similar to this issue nodejs/undici#702

@@ -83,9 +87,10 @@
"eslint-plugin-istanbul": "0.1.2",
"eslint-plugin-node": "11.1.0",
"express": "4.17.1",
"graphiql": "1.0.6",
"graphiql": "^1.4.1",
Copy link

@justinmchase justinmchase May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@justinmchase
Copy link

All of the failing tests for node 12 have the errors DeprecationWarning: The legacy HTTP parser is deprecated.

@junminstorage
Copy link
Contributor Author

junminstorage commented May 6, 2021

@justinmchase thank you for approving it.
right, it is documented here: #756.
@acao is it possible to continue to merge this PR and release it?

@justinmchase
Copy link

I agree it's probably something that can be fixed later. It appears that it would not create a regression.

@acao
Copy link
Member

acao commented Jun 1, 2021

@junminstorage let me know if there’s anything I can help with here! We will need to get this test passing though. Seems to be an issue with a fixed http mocking utility not being compatible with the latest node 12. Do you think you’ll need any more patches to GraphiQL?

@acao
Copy link
Member

acao commented Jun 1, 2021

@junminstorage ok we're good to merge here it looks like! thanks everyone for the awesome work, including @justinmchase for moving things along!

@junminstorage
Copy link
Contributor Author

@acao I fixed the mocha issue as stated in #762 , this PR is ready to go.

@acao acao changed the title Support graphql-ws client for GraphiQL IDE feat: support graphql-ws client for GraphiQL IDE Jun 1, 2021
@acao acao merged commit e02079c into graphql:main Jun 1, 2021
@acao
Copy link
Member

acao commented Jun 1, 2021

a note for everyone!

with this PR, you should now be able to use websockets with either the legacy or modern graphql-ws transport spec!

another PR must be merged for @stream and @defer support to become stable:
graphql/graphql-js#2839

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.

3 participants