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 support for inline fragments #65

Merged
merged 2 commits into from
Aug 28, 2021
Merged

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

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

Copy link
Contributor

@dnerdy dnerdy left a 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.

Comment on lines +348 to +366
fragmentFields, err := g.convertInlineFragment(
namePrefix, selection, containingTypedef, queryOptions)
Copy link
Contributor

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.)

Base automatically changed from benkraft.fragments-0 to main August 28, 2021 01:04
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
@benjaminjkraft benjaminjkraft merged commit e6b1984 into main Aug 28, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.fragments-1 branch August 28, 2021 01:06
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