GraphQLSwift / GraphQL

The Swift GraphQL implementation for macOS and Linux
MIT License
938 stars 72 forks source link

Schema Definition Language Support #154

Closed NeedleInAJayStack closed 4 days ago

NeedleInAJayStack commented 1 week ago

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:

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 https://github.com/GraphQLSwift/GraphQL/issues/55

paulofaria commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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.

NeedleInAJayStack commented 5 days ago

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 commented 5 days ago

Perfect! Awesome work, man! 😄