-
Notifications
You must be signed in to change notification settings - Fork 73
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
Schema Definition Language Support #154
Schema Definition Language Support #154
Conversation
e4f2317
to
560ac6f
Compare
The main difference between the canonical implementation and ours is the need for type references. If I recall correctly, it was because JS used thunks because it allows a symbol to be used inside a closure before its value is bound. I looked for other implementations for inspiration, like Java. I think that's where I got |
Oh thanks, that's very good background! In this PR I was able to get all existing GraphQL and Graphiti tests running without TypeReferences after changing the GraphQLObject
I marked it with Also, sorry - I know this is a humongous pull request. Unfortunately all these features use each other in their tests so it was hard to decouple them into separate pull requests. |
How would cyclical or recursive references be defined without About the PR size, no worries, I am famous for such PRs myself. 😂 |
560ac6f
to
6dde6dc
Compare
Sure, here's an example of a recursive type with TypeReference removed. This works out-of-the-box because For non-globals, you can do hit the issue of referencing a variable in a closure before it is created. This is solved by just creating the GraphQL type with empty fields, and then setting the Downstream in Graphiti, this then works exactly as you'd expect, where type references are no longer necessary to handle creation ordering/etc |
Oh, that's great news. I'm not sure if SSWG has any guidelines regarding deprecation. I imagine that as long as we follow SEMVER we're fine. So, if we're going to bump to a major version anyway, I'd say let's remove it altogether. Maybe just add some documentation somewhere explaining how to deal with its absence like you just explained. |
Extensions mean that definitions may initially be empty and extended later.
This is an execution no-op but allows SDL printing to order correctly.
Also removes codability of GraphQL types that now use closures, and removes TypeReference usage. Instead, use circular typing in Swift and Javascript approach where fields/interfaces are stored as closures and resolved realtime.
Matches with the reference implementation
This is necessary in order to compare object identifiers
6dde6dc
to
39fe70c
Compare
Aligns with graphql-js and supports Extension nodes
39fe70c
to
070e721
Compare
I looked through the docs here, but it doesn't appear they have any deprecation requirements, so it seems like we're just okay bumping major.
Great point. I've added a You mind giving it one more quick look? Thanks for the review! |
Perfect! Awesome work, man! 😄 |
This adds full Schema Definition Language (SDL) support to this package, including:
It does so primarily by porting graphql-js.
Unfortunately, this is a breaking change, even though I worked hard to keep the breakages to a minimum. Notably:
Definition
type properties have changed slightly (become optional or were converted to closures). I back-supported in most cases, but it wasn't possible everywhere.Since this is a breaking change, I'd be interested in opinions on whether we should just drop TypeReference support instead of keeping it around as
deprecated
. And while we're at it, should we make any other public interface changes?Fixes #55