GraphQLClient Support

[Sytten]

Hey there!
I built a prototype implementation for GraphQLClient (over here https://github.com/caido/graphql-ws-client).
Do you still plan on working on this lib, if so I will upstream my changes otherwise I will go my own way and most likely try to integrate it directly in GraphQLClient.

[obmarg]

Hey @Sytten - thats cool that you’ve managed to add graphql_client support. Would be happy to accept a PR if you want to contribute it.

[obmarg]

For a bit more detail around "working on this lib": I've not been especially active in this repo because I've not had a solid use case for subscriptions myself yet and cynic has been taking up most of my open source time lately. Will probably be something I focus on more eventually.

Built this as I wanted to have subscription support in cynic but figured it could be re-used for graphql_client and others so would be better off living outside of cynic. Happy if you want to fork & integrate it direct into graphql_client but I do think it'd be nice if all the ecosystem could benefit from any efforts made.

[Sytten]

Make sense. I agree that it would be nice to centralize efforts on websocket.

I was looking at alternatives yesterday and I found https://github.com/AircastDev/glimesh-rs which wraps the whole handle of lower level connections quite nicely for graphql-client. They use a custom websocket protocol though (but the implementation looks solid), so I am thinking I want to build the equivalent but for normal people. I can either build the WS part using this lib or do it custom made, so it mostly depends on what you decide to do here.

I also took a look at Gurkle WS implementation (https://github.com/technocreatives/gurkle/blob/main/crates/client/src/ws.rs) which has some nice ideas too.

As a first step I would:

• Remove all the prints
• Add tracing instead
• Dont use a fork of async executors (or upstream chages)
• Add a readme
• Probably split the cynic implementation into its own crate or module

Then eventually add the graphql-ws protocol, fix bugs, add documentation, etc.

[obmarg]

Ah, nice. Hadn't seen that glimesh thing - shame it's not very standard but looks nice.

I can either build the WS part using this lib or do it custom made, so it mostly depends on what you decide to do here.

On this front it's entirely up to you. I'd like there to be a library that implements the websocket layer for graphql clients but I am not that bothered if it ends up not being this one. Happy to accept contributions and provide feedback if you do want to work off of this though (and can sort out commit access for you after a couple of PRs - I don't want to be a blocker).

As a first step I would remove all the prints, add tracing, add a readme and probably split the cynic implementation into its own crate or module. Then eventually add the graphql-ws protocol, fix bugs, add documentation, etc.

Yeah, most of that sounds sensible. Removing the prints in particular. Didn't realise I'd left those in 😓

Happy for the cynic stuff to go into a module as well but probably slightly tricker to move it to a crate - likely to start running into fun with the orphan rule then.

Around graphq-ws can we clarify which protocol you mean? I thought I had implemented graphql-ws (although somewhat confusingly it refers to itself as graphql-transport-ws in the spec, which is fairly similar to the old apollo protocol subscriptions-transport-ws)

[obmarg]

Dont use a fork of async executors (or upstream chages)

Yeah, I tried to upstream my changes najamelan/async_executors#7 and unfortunately the async_executors maintainer disagreed with them. Wanted me to go and try to make some changes in tokio to accomodate the way he thought it should work. I kind of just gave up at that point. Although agree we should probably try to find a way forward on that.

[Sytten]

Unrelated but this fork added a WASM implementation which could be nice to incorporate: https://github.com/Yatekii/graphql-ws-client. Also they seem to have found a way to bring the executor up to date?

Ha I edited my comment at the same time, take a look a gurkle too.

Agreed for cynic, module is good.

Ha I was confused then, that is good we don't really need to support the old protocol anyway.

[Sytten]

I shopped around for other runtime agnostic libraries but they all seem to suffer from the same problem.

• https://github.com/bastion-rs/agnostik

I found this interesting post rust-lang/wg-async#45 (comment)
Could we use the futures::task::Spawn trait instead and leave the implementation to the user?

[obmarg]

Unrelated but this fork added a WASM implementation which could be nice to incorporate: https://github.com/Yatekii/graphql-ws-client. Also they seem to have found a way to bring the executor up to date?

Yeah, would be get that PR into a state that it can be merged. Unfortunately they've not fixed the executor thing, just ignoring it because tokio doesn't matter in a frontend wasm context: #30 (comment)

[obmarg]

Could we use the futures::task::Spawn trait instead and leave the implementation to the user?

Interesting. The async_executors implementations also implement this Spawn trait, so I think there might have been some reason it wasn't suitable. Although I absolutely can't remember what that problem was, which isn't much help. Does look like it'd be suitable and if there's a way we can make it work I'd definitely be up for it.

[obmarg]

Actually, having looked at it - futures::task::Spawn doesn't return a join handle, the future_executors trait does. Possible we could make do without that though - seems preferable to using a fork...

[Sytten]

I found the futures::task::SpawnExt which does return a join handle.
Pretty sure we could refactor to avoid using a handle but I will first try with the extension.
I am still searching if someone wrote code to implement that trait in tokio.

[Sytten]

Got it working: caido@8b14b5f