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
Transport ws protocol #355
Transport ws protocol #355
Conversation
…sport ws protocol
Thanks for your work. Obviously you put a lot of effort in this. I'll review it properly in the next days. |
@juancastillo0 I'm also interested by this feature. Is your PR in draft only because you need to fix the tests or are you going to make further changes? |
Hi! There are a couple of things that have to be done. Unfortunately, I will not be able to make the changes in the following weeks. If you need it urgently, I think you could take the things from the code that help you and finish the implementation. If you can wait, I will make the modifications in some time. I spoke about most of the changes in the previous comment. Thanks! |
Thanks for the work in the PR @juancastillo0! |
I have used these changes in a Ferry Graphql Dart application with a Apollo Server v4.0 running graphql-ws on the backend. This worked perfectly except for returning
Simply updating Map<String, Object?> toJson() => {"type": type.name}; instead of Map<String, Object?> toJson() => {"type": type.name, "payload": payload}; Hopes this helps someone else. |
@juancastillo0 I can give an assist to finish up the remaining items. I know you're busy with Leto and I can give a hand to polish up the rest. I was using graphql-ws as a test server and did see that @knaeckeKami is there anything you're waiting for besides the go-ahead? |
…nseParser and RequestSerializer
…dd logging function, pass originalStackTrace
For me the PR looks good, last time I checked it still had some debug prints in it, obviously that should be fixed and we need to make sure that the changes are backwards compatible in order to not break existing users. @agent3bood what do you think? I don't use websockets for graphql at the moment, you seem to do, what do you think? |
Thanks for the effort completing this long awaited feature. |
It's a different sub-protocol, the protocol is the same (websocket). |
Both subprotocols have some shared logic (messages.dart, exceptions, some typedefs). If we extract the transport-ws subprotocol in its own package, we either have to create a gql_websocket_common package for the shared code or duplicate some code. IMO in order to keep the number of packages manageable, it's justifiable to keep both links in the same package in this case (though I can see arguments for separate packages as well). The documentation should definitely mention the Subprotocol situation in any case. |
Any chance this could be reviewed soon? |
Yes, I agree we should wrap this up. I'm taking the decision to merge it into the existing gql-websocket package to avoid the maintenance overhead of 3 packages for graphql websockets. I hope that's okay for you @agent3bood . |
When should we hope a new release for this? |
The release process broke with the update to dart 3.0, I had to update the CI, sorry for the delay. |
Closes #305.
Hi this PR implements the
graphql-transport-ws
subprotocol. The code is a direct port of https://github.com/enisdenjo/graphql-ws/blob/master/src/client.ts with some changes to implement theLink
interface and replace some JavaScript patterns to what one would expect in Dart. There is still some work to be done, at the moment there are some TODO comments and print statements in the code, some of the comments reference the JavaScript API and the tests for the apollo part are failing. I think some timer is being executed multiple times when the expectAsyncN function only expects a single execution. When trying to reconnect to the server, for example. However, the core logic works and is fully tested most of the changes that need to be done are about the external API or cleaning up comments.I created a
_testLinks
function in the test file to test both subprotocols using the same logic. However, that makes it harder to test it in vscode since the extension does not recognize the test prefix associated with each subprotocol. One can use thedart test --name "transport ws sub-protocol <test-name>"
cli, but I am not sure if we should try to split it so it is easier to debug.At the moment, there are two separate
Link
implementations, with somewhat different parameters. Not sure what could be the best approach here, maybe we could try to unify some of the constructor parameters.When a message of type "error" is encountered, the request stream throws an error with the payload (a list of
GraphQLError
). The behavior is different from the apollo protocol where multiple errors can be sent as regular events within the stream. We could do the same, the exact line is here. However, I believe there would be no easy way to distinguish between a message of type "error" and type "next" with errors payload as an end user. According to the spec, the type "error" ends the request, no more messages should be sent.I have tested it end to end with a https://github.com/juancastillo0/leto server.
Thanks!