fix: Remove to_graphql and make the interpreter runtime a requirement for older GraphQL versions #177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daemonsy
changed the title
daemonsy
commented
Mar 8, 2022
| @@ -58,6 +58,11 @@ def top_products(first:) | |||
| end | |||
|
|
|||
| class ProductSchema < GraphQL::Schema | |||
| if Gem::Version.new(GraphQL::VERSION) < Gem::Version.new('1.12.0') | |||
There was a problem hiding this comment.
All the other test schemas already have this code block, these two just happened to be missed 🤷♂️
daemonsy
commented
Mar 8, 2022
| @@ -293,8 +293,9 @@ See discussion at [#74](https://github.com/Gusto/apollo-federation-ruby/issues/7 | |||
|
|
|||
| ## Known Issues and Limitations | |||
|
|
|||
| - Only works with class-based schemas, the legacy `.define` API will not be supported | |||
| - Does not add directives to the output of `Schema.to_definition`. Since `graphql-ruby` doesn't natively support schema directives, the directives will only be visible to the [Apollo Gateway](https://www.apollographql.com/docs/apollo-server/api/apollo-gateway/) through the `Query._service` field (see the [Apollo Federation specification](https://www.apollographql.com/docs/apollo-server/federation/federation-spec/)) or via [`Schema#federation_sdl`](https://github.com/Gusto/apollo-federation-ruby/blob/1d3baf4f8efcd02e7bf5bc7e3fee5b4fb963cd25/lib/apollo-federation/schema.rb#L19) as explained above. | |||
| - For GraphQL older than 1.12, the interpreter runtime has to be used. | |||
There was a problem hiding this comment.
I wonder if we should try to do a check in when the federation module is included. Given how old these versions are, I thought it wasn't necessary.
daemonsy
changed the title
|
I'd think we can remove support for GraphQL < 1.12 once we support 2.0. But I'd like to get some stats first if I can to confirm that they're not widely used by the community. |
rylanc
pushed a commit
that referenced
this pull request
Mar 8, 2022
## [2.2.1](v2.2.0...v2.2.1) (2022-03-08) ### Bug Fixes * Remove to_graphql and make the interpreter runtime a requirement for older GraphQL versions ([#177](#177)) ([bfc3082](bfc3082))
|
🎉 This PR is included in version 2.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This was referenced Mar 9, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's up?
Trying to deal with the problem caused by
to_graphqlin #151.The cause
GraphQL Ruby is deprecating the
to_graphqlmethod, slated for removal in 2.0. In 1.13, an argument was introduceddef to_graphql(silence_deprecation_warning: false).We have a
to_graphqlmethod in our codebase to add federation directives decorations. It is breaking because we defined it as a method with no arguments.Tests didn't break because we don't actually call this method in our specs. We rely on GraphQL ruby to call it in GraphQL versions where the interpreter runtime is not the default.
Complications
First thought was to add a spec to it, proxy arguments over. I'm used to proxying args
Ruby 2.7.xstyle. It failed in our specs because the specs are running onRuby 2.6.x. See this article.Given that Ruby 3.x is out, our keyword proxying should do the right thing. We also really didn't want to check for arity.
Let's just remove
to_graphqlSpecs fail if we remove the method, but why? The library requires the call to populate metadata on the legacy runtime.
If we made the interpreter runtime a requirement on older versions of GraphQL where it wasn't the default, all specs pass.
Given how old these versions are, I think this is the best solution. We're going to remove its use for 2.0 anyway.