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.

Sign up for GitHub

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

Jump to bottom

feat: Get Apollo Federation to work with GraphQL 1.13.x #160

Merged
merged 18 commits into from
Feb 4, 2022
Merged

feat: Get Apollo Federation to work with GraphQL 1.13.x #160

merged 18 commits into from
Feb 4, 2022

Conversation

daemonsy
Copy link
Contributor

@daemonsy daemonsy commented Jan 26, 2022 •
edited
Loading

This is a PR to gauge the effort to get the library working with GraphQL 1.13.

The flow and tasks:

  • Pre upgrade work or blockers
    • Remove GraphQL1.9.x support
    • Upstream specs that should pass in all versions
    • Fix semantic release issue that keeps changing google-protobuf to a platform specific version Semantic release bot changes our Gemfile.lock #169 This is a monster on its own :(
  • Merge required code changes
  • Discuss deprecations and removals (e.g. I think it's wise to remove support for GraphQL 1.9 at this point)
  • Test it in internal app
  • [ ] Release new major version of the gem (there's no breaking changes, in fact no code changes, so we should be good to release a minor version)

@daemonsy daemonsy changed the title [WIP] Get Apollo Federation to work with GraphQL 1.13.x [WIP] feat: Get Apollo Federation to work with GraphQL 1.13.x Jan 26, 2022
daemonsy
daemonsy commented Jan 26, 2022
View reviewed changes
Copy link
Contributor Author

@daemonsy daemonsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial self review

lib/apollo-federation/has_directives.rb Outdated
@@ -9,7 +9,7 @@ def add_directive(name:, arguments: nil)
@federation_directives << { name: name, arguments: arguments }
end

def to_graphql
def to_graphql(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self note: Check out the definition change in #to_graphql and how this method is used

gemfiles/graphql_1.13.gemfile.lock Outdated
ruby-debug-ide

BUNDLED WITH
2.1.4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use bundler 2.3.x

spec/apollo-federation/service_field_spec.rb Outdated
@@ -408,75 +406,64 @@ def self.visible?(context)
)
end

context 'with a filter' do
context 'context in schema generation' do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test broke in 1.13.x, because there's a new validation in GraphQL Ruby that warns against having an empty schema, but we were asserting that "Field '_service' doesn't exist on type 'Query'".

After looking at the original PR that introduced this, I think the intent is to make sure that Schema.federation_sdl respects context and uses it generation.

See https://graphql-ruby.org/authorization/visibility.html

I struggled with fixing it trying to get the right indirect error message across different GraphQL versions. Also, I think schema level filters are deprecated? I can't find docs on it anymore on graphql-ruby.

So with that, I rewrote the test to basically assert that we're using the context in SDL generation. Further improvements welcome.

@daemonsy daemonsy mentioned this pull request Jan 26, 2022
Closed
daemonsy
daemonsy commented Jan 28, 2022
View reviewed changes
Gemfile.lock Outdated
@@ -42,7 +42,7 @@ GEM
debase-ruby_core_source (0.10.9)
diff-lcs (1.3)
erubi (1.9.0)
google-protobuf (3.19.3-x86_64-linux)
google-protobuf (3.19.3-x86_64-darwin)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ on why there is the platform lock

@daemonsy daemonsy changed the title [WIP] feat: Get Apollo Federation to work with GraphQL 1.13.x feat: Get Apollo Federation to work with GraphQL 1.13.x Feb 1, 2022
@daemonsy daemonsy changed the title feat: Get Apollo Federation to work with GraphQL 1.13.x feat: Get Apollo Federation to work with GraphQL 1.13 Feb 1, 2022
@daemonsy daemonsy changed the title feat: Get Apollo Federation to work with GraphQL 1.13 feat: Get Apollo Federation to work with GraphQL 1.13.x Feb 1, 2022
This was referenced Feb 1, 2022
Merged
Merged
@daemonsy daemonsy requested review from geshwho and grxy February 4, 2022 14:55
geshwho
geshwho approved these changes Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@geshwho geshwho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@daemonsy
Copy link
Contributor Author

daemonsy commented Feb 4, 2022

@grxy and @geshwho [Public convo] I'm going to test this on an internal app, if it looks good and y'all are good with the code changes, will :shipit:.

Looking at the changes, it looks like it should be a minor bump, no breaking changes. Folks using GraphQL 1.13 will find it working correctly as expected.

@daemonsy daemonsy merged commit 800001b into main Feb 4, 2022
@daemonsy daemonsy deleted the graphql-1.13 branch February 4, 2022 16:25
rylanc pushed a commit that referenced this pull request Feb 4, 2022
@semantic-release-bot
chore(release): 2.2.0 [skip ci]
1d83203 
# [2.2.0](v2.1.0...v2.2.0) (2022-02-04)

### Features

* Get Apollo Federation to work with GraphQL 1.13.x ([#160](#160)) ([800001b](800001b)), closes [#147](#147)
@rylanc
Copy link
Contributor

rylanc commented Feb 4, 2022

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rylanc rylanc added the released label Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geshwho geshwho geshwho approved these changes

@grxy grxy Awaiting requested review from grxy

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daemonsy @rylanc @geshwho @1060ki