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

Schema Definition Language Support #154

Merged

Conversation

NeedleInAJayStack
Copy link
Member

@NeedleInAJayStack NeedleInAJayStack commented Oct 21, 2024

This adds full Schema Definition Language (SDL) support to this package, including:

  • Printing SDLs for existing schemas
  • Building new schemas directly from SDLs
  • Using SDL to extend already-defined schemas
  • Validating SDL schemas

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:

  • TypeReference was deprecated. Almost all existing usage is back-supported, but recursively defined types can be problematic. Most of users won't be impacted after we update Graphiti.
  • Some public 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

@NeedleInAJayStack NeedleInAJayStack self-assigned this Oct 21, 2024
@NeedleInAJayStack NeedleInAJayStack force-pushed the experiment/SDL-utilities branch 2 times, most recently from e4f2317 to 560ac6f Compare October 21, 2024 06:00
@paulofaria
Copy link
Member

paulofaria commented Oct 24, 2024

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 TypeReference from. When you say TypeReference was deprecated, what exactly do you mean. Maybe I'm misremembering stuff, though?

@NeedleInAJayStack
Copy link
Member Author

NeedleInAJayStack commented Oct 25, 2024

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 TypeReference from.

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 field property to a closure-based API. A pretty good example of the change can be seen in this comment diff: https://github.com/GraphQLSwift/GraphQL/pull/154/files#diff-a5342873cae3c47e54aedd945d19280fa6c7d03f66ad1f7ab13b16ec79c9acf1L288-L299 I'd be interested in your thoughts and if you see any gaps with that approach!

When you say TypeReference was deprecated, what exactly do you mean.

I marked it with @available(*, deprecated, ...) here. By doing so, it will give compiler warnings to anyone that is using it, but will still compile. I did this back when I thought I would be able to make this PR a minor version bump, but if it requires a major it seems like we maybe ought to just delete it.

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.

@paulofaria
Copy link
Member

How would cyclical or recursive references be defined without TypeReference? Can you point me to examples?

About the PR size, no worries, I am famous for such PRs myself. 😂

@NeedleInAJayStack
Copy link
Member Author

How would cyclical or recursive references be defined without TypeReference? Can you point me to examples?

Sure, here's an example of a recursive type with TypeReference removed. This works out-of-the-box because CharacterInterface is a global variable.

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 fields closure in a subsequent line. This function contains a test schema that uses this approach on a recursive type. Note that this has the added benefit of allowing the user to avoid the memory cycle caused by recursive types.

Downstream in Graphiti, this then works exactly as you'd expect, where type references are no longer necessary to handle creation ordering/etc

@paulofaria
Copy link
Member

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.

paulofaria
paulofaria previously approved these changes Oct 27, 2024
@NeedleInAJayStack
Copy link
Member Author

I'm not sure if SSWG has any guidelines regarding deprecation.

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.

just add some documentation somewhere explaining how to deal with its absence like you just explained.

Great point. I've added a MIGRATION.md file and linked it from the README, and I've removed GraphQLTypeReference and all usage.

You mind giving it one more quick look? Thanks for the review!

@paulofaria
Copy link
Member

Perfect! Awesome work, man! 😄

@NeedleInAJayStack NeedleInAJayStack merged commit 5e098b3 into GraphQLSwift:main Oct 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDL support
2 participants