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

fix: Make the tracer work with the new interpreter runtime #59

Merged
merged 6 commits into from
Mar 25, 2020

Conversation

daemonsy
Copy link
Contributor

What's up?

Context can be on the query on passed directly as denoted by this statement. By without passing a second argument to fetch, it raises so we never get to the data.fetch(:query).context branch.

@daemonsy daemonsy changed the title Default to nil to run the or conditional fix: Default to nil to run the or conditional Mar 20, 2020
Copy link
Collaborator

@grxy grxy left a comment

Choose a reason for hiding this comment

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

@daemonsy Looks good. Mind looking into writing a failing test to validate this change?

let(:base_schema) do
Class.new(GraphQL::Schema) do
use ApolloFederation::Tracing
RSpec.shared_examples 'a basic tracer' do
Copy link
Contributor Author

@daemonsy daemonsy Mar 25, 2020

Choose a reason for hiding this comment

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

Large diff to the spec mostly due to indentation and an edit on one of the line. I'll highlight where the changes are.

Right on this line, I wrapped the whole test into a shared example so we can run them with the interpreter on

error: [{
message: "Can't continue with this query",
location: [{ line: 1, column: 15 }],
json: %({"message":"Can't continue with this query","locations":[{"line":1,"column":15}],"path":["items",1,"name"]}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed some whitespace here. Previously it had some whitespace added into it.

field_type = data.fetch(:field).type.unwrap.graphql_name
field = data.fetch(:field)
field_name = field.graphql_name
field_type = field.type.unwrap.graphql_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call to write a spec @grxy . This has always been broken by the test caught it. The graphql_name of the type doesn't include whether it's nullable, hence the need to check the field

if data.include?(:context)
path = context.path
field_name = context.field.graphql_name
field_type = context.field.type.to_s
parent_type = context.parent_type.graphql_name
else # legacy runtime
else # interpreter runtime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we got the legacy and new interpreter the wrong way round. I get into this branch

@daemonsy
Copy link
Contributor Author

Hey @noaelad 👋 this is a small fix before we can switch our own stuff to use the new interpreter.

field_type = data.fetch(:field).type.unwrap.graphql_name
field = data.fetch(:field)
field_name = field.graphql_name
field_type = field.type.to_type_signature
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 seems like it's what we want. The other hacky way was leaky

Copy link
Collaborator

@grxy grxy left a comment

Choose a reason for hiding this comment

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

image

@daemonsy daemonsy changed the title fix: Default to nil to run the or conditional fix: Make the tracer work with the new interpreter runtime Mar 25, 2020
@grxy grxy merged commit de4caf0 into Gusto:master Mar 25, 2020
rylanc pushed a commit that referenced this pull request Mar 25, 2020
## [1.0.3](v1.0.2...v1.0.3) (2020-03-25)

### Bug Fixes

* Make the tracer work with the new interpreter runtime ([#59](#59)) ([de4caf0](de4caf0))
@rylanc
Copy link
Contributor

rylanc commented Mar 25, 2020

🎉 This PR is included in version 1.0.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants