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

SDL support #55

Open
mtvspec opened this issue Nov 4, 2019 · 16 comments
Open

SDL support #55

mtvspec opened this issue Nov 4, 2019 · 16 comments

Comments

@mtvspec
Copy link

mtvspec commented Nov 4, 2019

Question

Support executable schema based on schema definition language.

@paulofaria
Copy link
Member

paulofaria commented Nov 8, 2019

We don't have support yet. We're open to contributions!

@dfperry5
Copy link

@paulofaria - is this still open?

@NeedleInAJayStack
Copy link
Member

@dfperry5 Yeah, SDL support is still an outstanding area for improvement.

@dfperry5
Copy link

dfperry5 commented Nov 13, 2023

@NeedleInAJayStack - I'm interested in trying this out. Do you happen to know where in https://github.com/graphql/graphql-js they read in the SDL stuff? I'll keep looking through it myself.

@NeedleInAJayStack
Copy link
Member

@dfperry5 That would be awesome! I'm actually not too familiar with where SDL stuff lives in graphql-js. I believe it is built into the lanugage parser, and that our repo already has some of the tokens required to interpret SDL. But I imagine there are still some incompatibilities to figure out. Thanks!

@rex-remind101
Copy link

hey yall, i wrote a parser for graphql that supports everything needed for client side parsing in swift, meaning it parses much of what's available in the schema https://github.com/remind101/AutoGraphParser/ . this was after dealing with some frustration with the official C libgraphqlparser which wasn't being kept up-to-date (still on the 2016 spec actually), but what has come out of it i think is very easy to read and maintain (see Language.swift). anyway, my next plan which i want to execute on soon is full schema parsing of the entire language. after that I would actually very much want this to be inherited / integrated / subsumed by GraphQLSwift and i'd be happy to help maintain it, i have a vested interest in it. let me know how this all sounds, would love to collaborate here :)

@paulofaria
Copy link
Member

Hey, @rex-remind101! This sounds awesome. Love that you're using swift-parsing. I'm open to incorporating the parser in the library and implementing SDL support. I didn't have time to look, but what is the status in regards to testing?

@rex-remind101
Copy link

rex-remind101 commented Dec 22, 2023

there's a grip of unit tests. i'm not sure how complete they are, however, we use this parser to support our client code-generator on our app that is very very large with a lot of diverse graphql queries to say the least. also, the codegen is tested across queries in a somewhat diverse suite of schemas too (which implicitly tests the parser) https://github.com/remind101/AutoGraphCodeGen/tree/main/Tests/Resources

btw, if it yall are happy to integrate it i think it would be nice to have the parser as its own lib under the GraphQLSwift moniker. that way users could use it for other cases (like our codegen lib). I'm also hopeful that as the C++ interop story aligns on swift we can even have this as an option more universally for other languages via FFI etc too :)

@paulofaria
Copy link
Member

Yep! All of this sounds good to me, cc @NeedleInAJayStack. Guess we also have to see how this would fit with the SSWG incubation process. @adam-fowler, is there any rules regarding the maturity level of package dependencies? Would splitting up the parser dependency require it to be at the same maturity level as the main package?

@NeedleInAJayStack
Copy link
Member

Yeah, it looks very cool! Thanks for sharing @rex-remind101!

I had a few questions as I reviewed:

  • If I'm reading right, it looks like it does two things: Converts from introspection query results into a structured schema, and is able to parse client-side queries into constituent parts. Is that accurate? Would you say that it is focused mainly on client use cases, or is it also relevant to server-side operations?
  • How do you feel on how this parser compares with the GraphQL Package's Parser? Do you think that it would make sense to combine them, adopt only one, or keep them separate? Or does it serve completely different use-cases?

Overall, I'd love to see more Swift client functionality in the GraphQLSwift org. If we're imagining this as a server-side tool, we probably just need to do a little thinking about where it fits in.

Thanks again for sharing, and excellent work!

@rex-remind101
Copy link

  • If I'm reading right, it looks like it does two things: Converts from introspection query results into a structured schema, and is able to parse client-side queries into constituent parts. Is that accurate? Would you say that it is focused mainly on client use cases, or is it also relevant to server-side operations?

that is accurate as of current because the immediate need was to service a code generator on the client side. but i want to expand it to include parsing of the entire language so it can serve both client and server side.

  • How do you feel on how this parser compares with the GraphQL Package's Parser? Do you think that it would make sense to combine them, adopt only one, or keep them separate? Or does it serve completely different use-cases?

looking a bit over it it seems to more or less serve the same purpose. one core difference I notice is that a Node existential type is used which doesn't exist in the AutoGraphParser as of current, but i see no reason that couldn't be added if needed. the other core difference is operationally, AutoGraphParser uses the swift-parsing parser combinators library where the existing package's parser doesn't. i think there's some advantages there to readability/syntax and also just having a core set of combinators GraphQLSwift can leverage and not need to maintain.

Overall, I'd love to see more Swift client functionality in the GraphQLSwift org

I think the end-result of the parser being able to cover the whole GQL language could be used for both client and server. also after the fact i think i could draw from my experience to add a client library too, the existing one works well enough but I've wanted to do a bit of a rewrite of parts especially given async-await in swift. i could maybe redirect work towards GraphQLSwift org instead depending on technical direction :)

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Dec 22, 2023

Cool, thanks for your take on those questions.

Overall, I'm on board with bringing this package into the GraphQLSwift org.

However, I'd hesitate to replace the GraphQL parser with this implementation (or really any other one). The swift GraphQL package as a whole is basically a direct port of the graphql-js reference implementation (see the graphql-js parser for similarities). This approach has some significant benefits:

  1. As features are added over time to the spec & reference implementation, it is obvious where and how to add them to GraphQLSwift's package.
  2. When bugs occur, fixing them is often as simple as comparing the Swift code to the JavaScript and tweaking it to match.
  3. By duplicating the JavaScript testing we can easily ensure full feature parity with the existing spec & popular tools.
  4. We ensure that the performance considerations made within the larger JavaScript GraphQL community are reflected in the Swift implementation.
  5. By having the same structure and implementation patterns, the package is more approachable to people coming from GraphQL in different languages.

It also brings some downsides, mainly that it doesn't always take advantage of some unique Swift language features like ResultBuilders, strong typing, etc. The approach so far has been to add those features on top of GraphQL in separate packages like Graphiti.

If either of you think I'm off-base on this, please let me know.

Aside from that, I definitely think having this parser as a separate package seems totally reasonable, whether under the GraphQLSwift org or your own. I really do like the technical approach, and think it could be very useful in building GraphQL utilities in Swift!

@paulofaria
Copy link
Member

I mostly agree with all points, @NeedleInAJayStack. Since this issue is related to SDL support, I wonder what's missing from the existing parser to support it? Also how would SDL support look like with Graphiti, for example?

Regarding client libraries. I would love to create a client library, maybe based on Swift macros? I believe there is something we can do to make it possible to hand the client API a Graphiti schema and be able to get code completion and type safety (build errors) when writing result builder queries based on the types defined in the schema?

@rex-remind101
Copy link

rex-remind101 commented Dec 22, 2023

ok, that logic is reasonable for maintaining the current parser. i still wouldn't mind bringing AutoGraphParser under the GraphQLSwift moniker though i'd still intend it to be full fledged GQL parser (at some point in the near future) which i'm not sure if that would create some confusion for people? possibly the intention would be if someone wants something very swifty use the AutoGraphParser (though maybe we change the name then too)?

to hand the client API a Graphiti schema and be able to get code completion and type safety

in terms of a client library i'm not sure how you intend on fitting in Graphiti schema, but i think any sufficient client library would need to be able to support backend languages other than swift too meaning it would need to perform introspection directly for schema verification.

(might be time to move that topic to another conversation though)

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Dec 23, 2023

Since this issue is related to SDL support, I wonder what's missing from the existing parser to support it?

I think the GraphQL Parser already supports SDL. Check out this test. It doesn't really check that the SDL parses correctly though, so there might be some edge cases to track down. The package is definitely missing some standard utilities around printing Schemas to SDL, and some SDL validation rules.

Also how would SDL support look like with Graphiti, for example?

I think this one's a bit more tricky. I'll spend a little time over the next few days looking into it and get back to you.

@paulofaria
Copy link
Member

Great, @NeedleInAJayStack! Looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants