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

Add GraphQL::Execution::Lookahead#selections #1907

Merged
merged 7 commits into from
Oct 29, 2018

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Oct 17, 2018

This PR adds the ability to get all of the selections from a Lookahead. I decided to keep the implementation simple by just re-using the selection method instead of refactoring, let me know if that's alright 😄

Continuation of #1894

@ianks ianks force-pushed the lookahead-selections branch from 79026bd to 48f3e8f Compare October 17, 2018 18:45
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

👏 ! Thanks for taking this on! I added a couple comments about how it could be made more consistent, let me know what you think.

@ianks
Copy link
Contributor Author

ianks commented Oct 19, 2018

@rmosolgo addressed your comments, should be good now once CI passes. Thanks for the help!

@rmosolgo
Copy link
Owner

Cool, thanks for taking a look! What do you think of my suggestion in 8bb135c ? Effectively, it merges selections, so that

a { b } 
a { c } 

Is treated the same as

a { b c } 

The GraphQL runtime does this, too -- a is only executed once. This causes selections.map(&:name) to return only [:a], not [:a, :a], does that seem ok to you?

@rmosolgo rmosolgo mentioned this pull request Oct 19, 2018
19 tasks
@ianks
Copy link
Contributor Author

ianks commented Oct 19, 2018

Ahhh ok. What is the reasoning for only returning it once? I would think that they are different selections because they have different sub-selections, This could potentially cause issues when attempting to dig deeped into the sub-lookaheads.

ie.

query_ one = a { b } 
query_two  = a { c } 

query_one.selections.find('a').selections # => ['b']
query_one.selections.find('a').selections # => ['c']

@rmosolgo
Copy link
Owner

cause issues

I'm sorry, I don't quite see it. Could you help me understand what scenario you want the different AST nodes reflected in a lookahead? Like, when would you interact with a lookahead that you want to distinguish between these two GraphQL snippets:

# snippet 1, the client has requested data in two different selections, 
# even though they're merged during query execution 
query {
  users { totalCount } 
  users { name } 
end 
# Snippet 2, `totalCount` and `name` are part of the same selection
query {
  users { 
    totalCount 
    name 
  }
}

What I've done in 8bb135c is normalized in that case, so regardless of whether the client sends snippet 1 or snippet 2, it looks the same:

users_field = lookahead.selection("users")
p users_field.selections.map(&:name)
# [:total_count, :name]

☝️ consider that bit of Ruby code, perhaps inside a resolver. If lookahead didn't merge subselections, what output would you expect for that Ruby code in the case of GraphQL snippet 1 and GraphQL snippet 2 above?

@ianks
Copy link
Contributor Author

ianks commented Oct 19, 2018

This makes sense now, and is a much better way of dealing with the problem. Essentially, we can consider 'name' to be unique?

@rmosolgo
Copy link
Owner

👌 Bingo! .selections will never return two lookaheads with the same name.

Ok, caveat: there are still unhandled edge cases here with aliases. For example, I'm not really sure what this will return:

query {
  users { 
    totalCount 
    name 
    alsoName: name 
  }
}

Because Lookahead pays no mind to alias. Honestly, I'm not to worried about it, because it works ok for my own case. It suits me to cross that bridge when we get to it!

@rmosolgo rmosolgo added this to the 1.9.0 milestone Oct 29, 2018
@rmosolgo rmosolgo merged commit 4dbe410 into rmosolgo:master Oct 29, 2018
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.

2 participants