-
Notifications
You must be signed in to change notification settings - Fork 21
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 Subscriptions Support #23
Conversation
Sorry for being slow with getting a review on this PR, but here's my two cents. I think the simple implementation you did here is really nice / easy to understand. I'm playing with this locally now to see how the project gets affected by the different possible message types and whether or not how this client handles the message makes sense. For example, if the server sends the keep alive type Otherwise once we have a clearer understanding of what the endge cases are for this feature, then I think some more tests to document that would be helpful. |
I'm going through the Graphql specification docs right now as I'm also unsure whether we should be using the python graphql websocket protocol https://github.com/graphql-python/graphql-ws or the apollo graphql websocket protocol https://github.com/apollographql/subscriptions-transport-ws. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing this locally against the test server you had setup the suscribe logic seemed to work fine, but when testing it against another graphql server with a demo subscriotion resolver it seems to fail.
The documentation on how to start a graphql subscriptions isn't really clear, but from what I tell we need to implement a handshake logic in the subscribe function to make sure we're following the apollo protocol.
The following resources were helpful for understanding the subscription stuff more:
- https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md - Documentation on the different messages the client and server should be able to handle.
- https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/client.ts#L585 - the source code for the apollo ws client in Typescript which includes the case statement for how to handle each type of message.
Also if you want to test against the demo server I tried locally, I cloned this repository and ran the server https://github.com/nicolasdao/apollo-subscription-demo.
@steve148 Thanks for your guidance and sorry for the delay in addressing your feedback. I read through the material you shared and had a better understanding of the protocol. I've added the handshake logic that you mentioned. I've tested it against the demo server you tried and it seems to work properly. It also still works with the test server that I had set up as it did without the handshake logic. Below is the code snippet that I used for manual testing if you want to try on your own. import asyncio
from python_graphql_client import GraphqlClient
client = GraphqlClient(endpoint="ws://localhost:4000/subscriptions")
query = """
subscription {
messageAdded(channelId: 1) {
id
text
}
}
"""
asyncio.run(client.subscribe(query=query, handle=print)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now my turn to apologize for being slow on this PR (a month too slow). I've been having computer issues lately forcing me to work on a non-ideal machine.
Thanks for making the changes, tested it and it seems good 👍 It is super simple which also makes it really easy to understand. One note is that it currently does not have a graceful way to handle the graphql server closing a websocket / subscription. Assuming all things work well then this is ok, but in the future we may want to do a better job of alerting the process with a better message for the websocket shutting down.
Totally agreed! We can work on it together down the road. 😎 |
What kind of change does this PR introduce?
This PR adds support for GraphQL subscriptions to the client. (fix #4)
What is the current behavior?
The client only supports GraphQL queries and mutations.
What is the new behavior?
The client also supports GraphQL subscriptions.
This PR implements the
subscribe
method of theGraphqlClient
class. Thewebsockets
dependency is added to the project because GraphQL subscriptions are executed over WebSocket unlike GraphQL queries and mutations.It wasn't easy to find public APIs that support GraphQL subscriptions. So, I wrote a very simple API myself for understanding how GraphQL subscriptions work and also for performing some manual testing.
The above screenshot is what I captured from the network tab of Chrome Developer Tools after initiating the
messageAdded
subscription. Once the endpoint receives a request in the form of{"type": "start", "id": "1", "payload": {"query": ..., "variables": ...}}
, it sends back with a series of responses like{"type": "start", "id": "1", "payload": {"data": ...}}
.My implementation in this PR is simply based on these behaviors between the client and the server that I observed on the Apollo Playground tool. There must be huge room for improvement but I hope this serves as the first step in enabling the client to support all GraphQL operations.
Here're the manual testing results.
Does this PR introduce a breaking change?
No. This doesn't touch any of the existing functionalities.
Other information