Expand the flatten option to support fields #30
Assignees
Labels
enhancement
New feature or request
help wanted
Issues that anyone could pick up and implement if useful to them
Comments
|
This would be especially useful for named fragments, where you might do f { ...MyFragment }and it would be great if we could just generate a field (Edit: this part, but not the original idea, is implemented as |
benjaminjkraft
added
enhancement
|
This has come up quite a lot with fragments. I'm going to try to work on it if I get time. |
benjaminjkraft
added a commit
that referenced
this issue
Sep 23, 2021
We ask for `__typename` on interface-typed fields so that we know which type to use when unmarshaling. We never strictly need this for fragments, because the context into which they're spread must either have `__typename` or must be of concrete type. But a few cases have come up where it would be somewhat convenient: first, if you're using genqlient's types for mocking (not really supported right now, but possible); and second, for consistency if we add an option to "flatten" fragments (#30), which would effectively drop the `__typename` field from the structs otherwise. Neither is a deeply compelling reason, I feel, but it's not too hard to do so we may as well. Test plan: make check Reviewers: marksandstrom, steve, jvoll, adam, miguel, mahtab
This was referenced Sep 23, 2021
benjaminjkraft
added a commit
that referenced
this issue
Sep 29, 2021
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
|
#121 implements this, but only for fragment spreads, as they've proven to be both the more useful case and by far the easier one to implement. See the PR for details. |
benjaminjkraft
changed the title
benjaminjkraft
added a commit
that referenced
this issue
Sep 30, 2021
## Summary:
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
Author: benjaminjkraft
Reviewers: csilvers, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo, StevenACoffman
Required Reviewers:
Approved By: csilvers, dnerdy
Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint
Pull Request URL: #121
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
If you do
mutation { MyMutation { error { code } } }, or something, we should be able to just return(code string, err error), at least if you ask us too. This would be a nice convenience -- the helper's callers no longer have to know as much about the query structure -- but the rules and logic to do it are surprisingly subtle.The text was updated successfully, but these errors were encountered: