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

Implementation-independent type of the query field in the subscribe message #44

Closed
Blemicek opened this issue Nov 1, 2020 · 11 comments · Fixed by #45
Closed

Implementation-independent type of the query field in the subscribe message #44

Blemicek opened this issue Nov 1, 2020 · 11 comments · Fixed by #45
Labels
enhancement New feature or request question Further information about the library is requested released Has been released and published

Comments

@Blemicek
Copy link

Blemicek commented Nov 1, 2020

Hello,
we have been looking for an alternative protocol to subscriptions-transport-ws and found this repository which looks very promising and far more mature.

The only problem is the query type in the subscribe message that seems to be javascript specific as the DocumentNode is part of internal classes of graphql-js, and our server is written in Java. Do you plan to make the protocol more open for other languages that use only the string query representation of GQL query in GQL library public API?

@n1ru4l
Copy link
Contributor

n1ru4l commented Nov 1, 2020

As far as I understand the query can either be a string or DocumentNode. So just implementing it in Java and expecting a string should be fine? Also I guess it wouldn't be that hard to write a function for decoding/verifying that the query is a valid JSON DocumentNode. I guess some graphql Java library already has such a function? The execution engine will most likely transform the string into a AST document anyways.

E.g. in graphql-js you have the graphql and execute function. The former is a pre-defined function you would call with a string document and it takes care of the parsing and executing. However, most execution transports (express-graphql, graphql-ws) use the execute function directly which takes a DocumentNode instead of a string and does no validation. This pushes the parsing and validation to user-land, but allows much more flexibility.

@enisdenjo enisdenjo added the question Further information about the library is requested label Nov 1, 2020
@enisdenjo
Copy link
Owner

enisdenjo commented Nov 1, 2020

Hey,

I am glad you found the Protocol helpful! Would be awesome to see an implementation in Java.

As mentioned in the comment before, the Protocol does indeed allow a string to be passed as the query argument for the Subscribe message. You should be able to tackle the parsing server-side.

Nevertheless, the query GraphQL Document type is meant to describe a complete file or request string operated on by a GraphQL service or client.

@Blemicek
Copy link
Author

Blemicek commented Nov 1, 2020

Well, if we decide to use this protocol in public API, we have to support both types because it's up to a client to choose which type it will use.

And yes, it's probably possible to parse the whole JS AST from JSON and transforms it into Java AST representation, but in that case, we have to fork GraphQL java as there is no public API that expects query AST as input (or find some way how to safely use internal API or serialize it as a query string on the server-side which is a really bad solution). So, it's cheaper for us to implement yet another Websockets subprotocol than to maintain a fork of the Java GraphQL ecosystem.

Anyway, thanks for the clarification on why the protocol use DocumentNode. I personally don't think that there is much performance benefit given by sending serialized AST into JSON then into the query string format as there must be some kind of validation anyway and the query format is more compact. I'm just curious, do you have any benchmarks?

Thanks again.

@enisdenjo
Copy link
Owner

enisdenjo commented Nov 1, 2020

The client should be able to decide between the two types. Some like sending the DocumentNode, like Apollo Client for example. So, I'd like the spec to be convenient enough to support such decisions.

Actually, I haven't done any benchmarks, yet. I do indeed prefer the string query as it reads much nicer and will often be much smaller in size (compared to a serialised Document type).

Furthermore, as this Protocol is aiming to be a part of the GraphQL standard, you're insights would be very welcome at the GraphQL over WebSocket PR in the official GraphQL over HTTP Proposal spec. Would be great to discuss more about the query type there as well.

@Blemicek
Copy link
Author

Blemicek commented Nov 1, 2020

Are sure about the Apollo client? It seems to me that it sends the string representation over HTTP:

I will check the link. It sounds interesting, thanks for pointing it out.

@enisdenjo
Copy link
Owner

enisdenjo commented Nov 1, 2020

It sure does, but only for HTTP though. Check out the WebSocket link implementation.

Thinking about it further, I must say that changing the query to be only of string type became much more appealing. Parsing the query server side would always be the preferred way and I wouldn't say that anyone would willingly type out a Document node for a request on the client side.

@Blemicek
Copy link
Author

Blemicek commented Nov 1, 2020

I know. It's up to a link implementation. And yes, I pointed HTTP link, because the only official (ok, semi-official, Apollo crew don't want to maintain it) WebSocket link implementation, I'm aware of, is the subscriptions-transport-ws which still use the string representation.

Please let me know how you decided, I'm really looking forward to it.

@enisdenjo
Copy link
Owner

I have made #45, but I am still not completely on board... There are some pros and cons in the description there.

Would be cool if you could make this argument at the GraphQL over WebSocket PR. Hearing more opinions would be a smart move when faced with making a decision like this.

@Blemicek
Copy link
Author

Blemicek commented Nov 1, 2020

Thank you for the really quick implementation and feedback to my question. I made a comment to the PR, so let see what the others think about it.

@n1ru4l
Copy link
Contributor

n1ru4l commented Nov 1, 2020

Unless the AST is not part of the GraphQL spec the DocumentNode support could be an experimental feature supported by the graphql-ws implementation that would allow certain performance optimizations.

@enisdenjo
Copy link
Owner

🎉 This issue has been resolved in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information about the library is requested released Has been released and published
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants