Document and improve support for binding non-scalars to a specific type #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: We had this setting called "scalars", which said: bind this GraphQL type to this Go type, rather than the one you would normally use. It's called that because it's most useful for custom scalars, where "the one you would normally use" is "error: unknown scalar". But nothing ever stopped you from using it for a non-scalar type. I was planning on removing this functionality, because it's sort of a rough edge, but a discussion with Craig found some good use cases, so instead, in this commit, I document it better and add some slightly nicer ways to specify it. Specifically, here are a few potential non-scalar use cases: - bind a GraphQL enum to a nonstandard type (or even `string`) - bind an input type to some type that has exactly the fields you want; this acts as a sort of workaround for issues #14 and #44 - bind an object type to your own struct, so as to add methods to it (this is the use case Craig raised) - bind an object type to your own struct, so as to share it between multiple queries (I believe named fragments will address this case better, but it doesn't hurt to have options) - bind a GraphQL list type to a non-slice type in Go (presumably one with an UnmarshalJSON method), or any other different structure The latter three cases still have the sharp edge I was originally worried about, which is that nothing guarantees that the fields you request in the query are the ones the type expects to get. But I think it's worth having the option, with appropriate disclaimers. The main change to help support that better is that you can now specify the type inline in the query, as an alternative to specifying it in the config file; this means you might map a given object to a given struct, but only in some cases, and when you do you have a chance to look at the list of fields you're requesting. Additionally, I renamed the config field from "scalars" to "bindings" (but mentioned it in a few places where you might go looking for how to map scalars, most importantly the error message you get for an unknown (custom) scalar). While I was making a breaking change, I also changed it to be a `map[string]<struct>` instead of a `map[string]string`, because I expect to add more fields soon, e.g. to handle issue #38. Finally, since the feature is now intended/documented, I added some tests, although it's honestly quite simple on the genqlient side. Test Plan: make tesc Reviewers: csilvers, marksandstrom, adam, miguel
benjaminjkraft
requested review from
aberkan,
csilvers,
dnerdy and
MiguelCastillo
csilvers
approved these changes
Aug 27, 2021
generate/config.go
Outdated
| // A TypeBinding represents a Go type to which genqlient will bind a particular | ||
| // GraphQL type. See Config.Bind, above, for more details. | ||
| type TypeBinding struct { | ||
| // The fully-qualified name of the Go type to which to bind. |
There was a problem hiding this comment.
Can you give an example of what a fully-qualified type-name looks like?
| // want, subject to the caveats described in Config.Bindings.) Local | ||
| // bindings are checked in the caller (convertType) and never get here. | ||
| globalBinding, ok := g.Config.Bindings[def.Name] | ||
| if ok && options.Bind != "-" { |
There was a problem hiding this comment.
You may want to comment that the options.Bind check is to allow a local option to override applying the global option.
generate/genqlient_directive.go
Outdated
| // genqlient-generated type. | ||
| // | ||
| // The value should be the fully-qualified type name, or e.g. | ||
| // []path/to/my.Type if the GraphQL type is [MyType]. (This allows |
There was a problem hiding this comment.
Can you make this example more concrete? Like, should the example be:
"[]github.com/Khan/genqlient/models/types.MyType"
Or is it just []models/types.MyType, or something else?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use. It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar". But nothing ever
stopped you from using it for a non-scalar type. I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.
Specifically, here are a few potential non-scalar use cases:
string)this acts as a sort of workaround for issues Improve options-handling for input types #14 and Omitempty-style handling for array elements #44
(this is the use case Craig raised)
multiple queries (I believe named fragments will address this case
better, but it doesn't hurt to have options)
with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get. But I think
it's worth having the option, with appropriate disclaimers.
The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.
Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar). While I was making a breaking change, I also changed
it to be a
map[string]<struct>instead of amap[string]string,because I expect to add more fields soon, e.g. to handle issue #38.
Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.
Test plan:
make tesc