Transport ws protocol

[juancastillo0]

Closes https://github.com/gql-dart/gql/issues/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 the Link 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 the dart 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!

[knaeckeKami]

Thanks for your work. Obviously you put a lot of effort in this. I'll review it properly in the next days.

[GP4cK]

@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? I can try to help you fix the test if it's the only thing left... Cheers

[juancastillo0]

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!

[robertoestivill]

Thanks for the work in the PR @juancastillo0! Looking forward to see this in the library soon!

[krejko]

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 payload on the ping causes a disconnection with the following errors.

errorOrClosed LikeCloseEvent(code: 4400, reason: Invalid message received, wasClean: null)
js_primitives.dart:30 shouldRetryConnectOrThrow LikeCloseEvent(code: 4400, reason: Invalid message received, wasClean: null)

Simply updating PingMessage's toJson to omit the payload attribute allows the client to stay connected to the server.

  Map<String, Object?> toJson() => {"type": type.name};

instead of

  Map<String, Object?> toJson() => {"type": type.name, "payload": payload};

Hopes this helps someone else.

[warrenisarobot]

@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 null payloads were causing protocol errors. After addressing that I was able to send and receive messages.

@knaeckeKami is there anything you're waiting for besides the go-ahead?

[knaeckeKami]

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?

[agent3bood]

Thanks for the effort completing this long awaited feature. I would make this as its own package instead of combining it with gql_websocket_link, it is a different protocol so it deserve its own package.

[sbatezat]

Thanks for the effort completing this long awaited feature. I would make this as its own package instead of combining it with gql_websocket_link, it is a different protocol so it deserve its own package.

It's a different sub-protocol, the protocol is the same (websocket). If you make that choice, you should rename the current package, because gql_websocket_link is not mentionning the sub-protocol used, only the protocol (websocket). Moreover, the current sub-protocol used is deprecated so I think it should be deprecated from gql_websocket_link too, and one day or another, be removed...

[knaeckeKami]

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.

[sbatezat]

Any chance this could be reviewed soon? The associated issue has more than one year and, as said previously, the sub-protocol currently supported - the only one - is deprecated. It makes this package (gql_websocket_link) entirely useless for modern apps.

[knaeckeKami]

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 .

[sbatezat]

When should we hope a new release for this?

[knaeckeKami]

The release process broke with the update to dart 3.0, I had to update the CI, sorry for the delay. gql_websocket_link 1.1.0 is released.