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 "flattening" fragment-spreads #121

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

One common use of fragment spreads is as the entirety of a field's
selection, e.g.

query MyQuery {
  myField {
    ...MyFragment
  }
}

In this case, by default, genqlient generates a wrapper type
MyQueryMyFieldMyType, which just embeds MyFragment. This makes
sense if you later want to add more fields in addition to the fragment
spread. But if you don't -- and you did the fragment because you want
to share types, it's an extra layer of indirection. (Which becomes
especially onerous if myField has list type ([MyType!]), such that
it's not just an extra attribute-access to get to MyFragment.)

The new option # @genqlient(flatten: true) simplifies this situation:
if applied to myField is skips the wrapper type;
MyQueryResponse.MyField will simply have type MyFragment (or
[]MyFragment, or whatever). This should hopefully make the typename
option, which has more limitations, less necessary.

Note that in #30 the initial idea was to support this for fields as
well. This would require significant additional complexity in the
JSON-(un)marshaling code, and has proven less necessary, so I
implemented this option only for fragment-spreads for now. With that
restriction, it was shockingly simple; we have to hook into a bunch of
different places, but they're all quite simple, since the structure of
the Go types still matches the structure in GraphQL.

Issue: #30

Test plan:

make check

One common use of fragment spreads is as the entirety of a field's
selection, e.g.
```graphql
query MyQuery {
  myField {
    ...MyFragment
  }
}
```
In this case, by default, genqlient generates a wrapper type
`MyQueryMyFieldMyType`, which just embeds `MyFragment`.  This makes
sense if you later want to add more fields in addition to the fragment
spread.  But if you don't -- and you did the fragment because you want
to share types, it's an extra layer of indirection.  (Which becomes
especially onerous if `myField` has list type (`[MyType!]`), such that
it's not just an extra attribute-access to get to `MyFragment`.)

The new option `# @genqlient(flatten: true)` simplifies this situation:
if applied to `myField` is skips the wrapper type;
`MyQueryResponse.MyField` will simply have type `MyFragment` (or
`[]MyFragment`, or whatever).  This should hopefully make the `typename`
option, which has more limitations, less necessary.

Note that in #30 the initial idea was to support this for fields as
well.  This would require significant additional complexity in the
JSON-(un)marshaling code, and has proven less necessary, so I
implemented this option only for fragment-spreads for now.  With that
restriction, it was shockingly simple; we have to hook into a bunch of
different places, but they're all quite simple, since the structure of
the Go types still matches the structure in GraphQL.

Issue: #30

Test plan:
make check

Reviewers: steve, csilvers, marksandstrom, mahtab, adam, miguel, jvoll
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

I'm excited to see this! so I'll approve it, but I don't think I'd notice if I missed something.

The logic is straightforward enough, as you point out; what I don't have a good handle on is if there's another place you need to add the flatten-check besides the 4 (!) you have now. I kinda-understand the first three of them: a containing query, struct, or interface/union. But I don't really understand the last one: if we're already doing the check on the container, why do we need to do the check on the fragment-spread itself? Is this for cases where we have nested fragment-spreads or something?

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.

Makes sense to me. 👍

docs/FAQ.md Outdated
@@ -319,7 +319,21 @@ query GetMonopolyPlayers {

and you can even spread the fragment into interface types. It also avoids having to list the fields several times.

Alternately, if you always want exactly the same fields, you can use the simpler but more restrictive genqlient option `typename`:
**Fragments, flattened:** the field `Winner`, above, has type `GetMonopolyPlayersGameWinnerUser` which just wraps `MonopolyUser`. If we don't want to add any other fields, that's unnecessary! Instead, we could do
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Winner" => "winner"

Also, the "winner" field directly above this section does have another field "winCount", though I understand this is referencing the query above that.

@benjaminjkraft
Copy link
Collaborator Author

The logic is straightforward enough, as you point out; what I don't have a good handle on is if there's another place you need to add the flatten-check besides the 4 (!) you have now. I kinda-understand the first three of them: a containing query, struct, or interface/union. But I don't really understand the last one: if we're already doing the check on the container, why do we need to do the check on the fragment-spread itself? Is this for cases where we have nested fragment-spreads or something?

The check is on the fragment-definition -- which is indeed for nested fragment-spreads, namely where you do e.g.

# @genqlient(flatten: true)
fragment Outer on T {
  ...Inner
}

in which case Outer effectively becomes an alias for Inner. (Actually we don't even bother to define it, although doing all the flattening as type aliases would be an interesting idea.) I'm not sure why you'd do that, but I figured there's no reason not to allow it.

- fix docs
- comment about fragments
- weaken validation on fragments, to match operation, where it applies
  to anything valid within the fragment
@benjaminjkraft benjaminjkraft merged commit c6d087c into main Sep 30, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.flatten branch September 30, 2021 00:52
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.

3 participants