-
Notifications
You must be signed in to change notification settings - Fork 115
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 support for inline fragments #65
Conversation
b6a3089
to
a7b1580
Compare
c54143a
to
10bb403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks solid to me. I don't know if I explicitly mentioned it before, but I really appreciate the testing infrastructure you've set up.
fragmentFields, err := g.convertInlineFragment( | ||
namePrefix, selection, containingTypedef, queryOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: it wasn't clear to me at first that this function returns nil for fragments that don't match and therefore the fragments aren't included. (Though it became clear after reading the functions below.)
47014d5
to
857294f
Compare
In this commit I add support for inline fragments (`... on MyType { fields }`) to genqlient. This will make interfaces a lot more useful! In future commits I'll add named fragments, for which we'll generate slightly different types, as discussed in DESIGN.md. In general, implementing the flattening approach described in DESIGN.md was... surprisingly easy. All we have to do is recurse on applicable fragments when generating our selection-set. The refactor to selection-set handling this encouraged was, I think, quite beneficial. It did reveal two tricky pre-existing issues. One issue is that GraphQL allows for duplicate selections, as long as they match. (In practice, this is only useful in the context of fragments, although GraphQL allows it even without.) I decided to handle the simple case (duplicate leaf fields; we just deduplicate) but leave to the future the complex cases where we need to merge different sub-selections (now #64). For now we just forbid that; we can see how much it comes up. The other issue is that we are generating type-names incorrectly for interface types; I had intended to do `MyInterfaceMyFieldMyType` for shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead I did `MyFieldMyType`, which is inconsistent already and can result in conflicts in the presence of fragments. I'm going to fix this in a separate commit, though, because it's going to require some refactoring and is irrelevant to the main logic of this commit; I left some TODOs in the tests related to this. Issue: #8 Test plan: make check Reviewers: marksandstrom, adam, miguel
857294f
to
28d7420
Compare
Summary:
In this commit I add support for inline fragments
(
... on MyType { fields }
) to genqlient. This will make interfaces alot more useful! In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.
In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy. All we have to do is recurse on applicable
fragments when generating our selection-set. The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.
One issue is that GraphQL allows for duplicate selections, as long as
they match. (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64). For now we just forbid that; we can see how
much it comes up.
The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do
MyInterfaceMyFieldMyType
forshared fields and
MyImplMyFieldMyType
for non-shared ones, but insteadI did
MyFieldMyType
, which is inconsistent already and can result inconflicts in the presence of fragments. I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.
Issue: #8
Test plan:
make check