Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graphql-ws Protocol Support #1622

Closed
casey-chow opened this issue Jan 14, 2021 · 20 comments · Fixed by #2168
Closed

graphql-ws Protocol Support #1622

casey-chow opened this issue Jan 14, 2021 · 20 comments · Fixed by #2168
Assignees
Labels
apollo-websockets help wanted Issues the Apollo team would love help from the community about

Comments

@casey-chow
Copy link

Feature request

Enable support for the graphql-ws protocol so servers can leverage the security and maintenance improvements.

Motivation

subscriptions-transport-ws has fallen into disrepair in recent years, and its successor, graphql-ws, introduces a completely new protocol which would need to be implemented for compatibility. I've been seeing a lot of issues with the server library and would like not to fork subscriptions-transport-ws 😅

@designatednerd
Copy link
Contributor

Can you help me understand what changes to our existing websocket library would be necessary for this? I know we're working on trying to figure some things out with WebSockets, but I think if there's a way to make this doable without having to build a completely parallel architecture we should at least consider it.

If it does require a completely parallel architecture that's a bit of a different question, though.

@casey-chow
Copy link
Author

casey-chow commented Jan 22, 2021

Here's a quick'n'dirty mapping of the two protocols based on their protocol documents:

Direction subscriptions-transport-ws graphql-ws
client -> server GQL_CONNECTION_INIT connection_init
server -> client GQL_CONNECTION_ACK connection_ack
server -> client GQL_CONNECTION_ERROR Closes the socket.
client -> server GQL_START subscribe
server -> client GQL_DATA next
server -> client GQL_ERROR error
bidirectional GQL_COMPLETE (server -> client)
GQL_STOP (client -> server)
complete
server -> client GQL_CONNECTION_KEEP_ALIVE Native websocket ping/pong.

I think a lot of the differences are in the nuances--graphql-ws tries to be more robust in its protocol design so there are definitely details not included here, but for the most part the protocols are fairly similar.

@designatednerd
Copy link
Contributor

OK. The next step would probably be finding a GraphQL-ws server we can write tests against, since our sample server is using Apollo Server and therefore subscriptions-transport-ws. Do you have any thoughts on how that would work to set up?

@casey-chow
Copy link
Author

If you point me towards the existing server I'd be more than happy to put together the graphql-ws version in a fork!

@designatednerd
Copy link
Contributor

@danabrooks
Copy link

Has there been any progress on this? I am very interested in graphql-ws support for Apollo on iOS.

@designatednerd
Copy link
Contributor

I'm dealing with other stuff at the moment - @casey-chow Were you ever able to get a look at this?

@designatednerd designatednerd added the help wanted Issues the Apollo team would love help from the community about label Mar 10, 2021
@dchappelle
Copy link

I am very interested in graphql-ws support for Apollo for Android.

@designatednerd
Copy link
Contributor

Hi @dchappelle, I'd recommend checking out the Android repo's issues and opening an issue if you can't find one - this is the iOS repo 😇.

@VladMelnik
Copy link

Has there been any progress on graphql-ws support for Apollo on iOS?

@phanikavi
Copy link

Any updates on this feature?

It works for me with these basic changes. graphql-ws-patch.txt

Apollo Android seems to have already implemented it in v3.0.0-alpha01 release.

@calvincestari
Copy link
Member

Hi 👋🏻

  • graphql-ws support is not something we're directly working on for the 1.0 release
  • we are always happy to review community contributions though so if you've got a working solution please consider submitting a PR and the team will review it

@hwillson hwillson added 2022-01 and removed 2022-01 labels Jan 20, 2022
@calvincestari calvincestari added this to the Release 0.51.0 milestone Jan 31, 2022
@phanikavi
Copy link

phanikavi commented Feb 4, 2022

Hi @calvincestari, We have been using the changes in the patch that I mentioned earlier to support graphql-ws and it is working fine. I see that this feature is now scheduled for 0.51.0 milestone. Please let me know if you need any help in verifying your changes.

@calvincestari
Copy link
Member

I see that this feature is now scheduled for 0.51.0 milestone. Please let me know if you need any help in verifying your changes.

That's correct @phanikavi. We're storming towards an imminent 1.0 alpha release so the graphql-ws work is slightly delayed. I think the patch will be a great starting point though. We'll get to it next week, thanks for the offer to verify changes.

@calvincestari
Copy link
Member

@phanikavi I've just created PR #2168 if you're still interested in verifying the changes with your environment.

@phanikavi
Copy link

awesome, @calvincestari these changes work fine in our environment.

@CureleaAndrei
Copy link

I think that the GQL_STOP is not mapped to complete

CONTEXT:
I've been implementing in our app the new graphql_transport_ws but encountered the following error when terminating a subscription:
"websocket is disconnected: WSError(type: Apollo.WebSocket.WSError.ErrorType.protocolError, message: \"Invalid message received\", code: 4400)"

After digging in I saw that the server get's a message of type: stop but in the graphql_ws protocol it should be complete. I think that the issue lies inside WebSocketTransport unsubscribe method which always sends an OperationMessage of type stop. Shouldn't it check the protocol and send it with type stop or complete depending on the protocol used ?

@calvincestari
Copy link
Member

I'll dig into this this week @CureleaAndrei.

@CureleaAndrei
Copy link

Thanks!

@calvincestari
Copy link
Member

@CureleaAndrei, sorry this took a bit longer than I had planned. PR #2320 is up which should fix the behaviour. Please give it a test if you've got the time - thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apollo-websockets help wanted Issues the Apollo team would love help from the community about
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants