59b6df6aabdf79cca684dd709fa455b5489352ff
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ben KraftandGitHub
|
3a5bf46da4 |
Add getter methods to all fields, not just where needed (#126)
## Summary: If your type implements an interface, we add getter methods for the shared fields, so that those may be accessed via the interface. But it turns out occasionally it's useful to have these getter methods when they don't implement a GraphQL interface, so you can use two genqlient-generated types in the same function if they have the same fields. (This comes up most often when you have a GraphQL union that maybe should really be an interface, or if you don't yet support interfaces implementing other interfaces (indeed our parser doesn't either). But one can imagine other use cases.) We can't predict how you want to do that, so we can't generate the interface, but we can generate the methods, so you can define the interface and do a type assertion from there. Since these methods are pretty simple to generate, we just do it always. (As with #120, if binary size becomes an issue we could later add an option to only generate methods that are truly needed but including them seems like the better default.) This also fixes a subtle and rare bug, which would have become much more common (indeed existing tests caught it). Specifically, if you have a query like ```graphql fragment FragmentOne on T { id } fragment FragmentTwo on T { id } query Q { f { # interface type T ...FragmentOne ...FragmentTwo } } ``` since both `FragmentOne` and `FragmentTwo` request some common field, say `id`, we generate a method `GetId` on each one. But since `FragmentOne` and `FragmentTwo` are both on `T`, we also include their interfaces in the interface we generate for the type of `f`, `QFT`. So `QFT` includes a method `GetId`. But on the implementations, the two methods conflict, and neither gets promoted; this causes various code to fail to compile. With this change, this would have happened much more frequently -- even if only one of the two fragments is on `T`, as long as both request the field. Anyway, we now generate explicit methods on each struct for all of its recursively emebedded fields -- using the logic from #120 to compute them -- so that we don't need to rely on method-promotion. ## Test plan: make tesc 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: https://github.com/Khan/genqlient/pull/126 |
||
|
Ben KraftandGitHub
|
5995653583 |
Refactor argument-handling to use a struct (#103)
## Summary: In this commit I refactor the argument-generation logic to move most of the code out of the template and into the type-generator. This logic predates #51, and I didn't think to update it there, but I think it benefits from similar treatment, for similar reasons. Specifically, the main change is to treat variables as another struct type we can generate, rather than handling them inline as a `map[string]interface{}`. Users still pass them the same way, but instead of putting them into a `map[string]interface{}` and JSONifying that, we generate a struct and put them there. This turns out to simplify things quite a lot, because we already have a lot of code to generate types. Notably, the omitempty code goes from a dozen lines to basically two, and fixes a bug (#43) in the process, because now that we have a struct, `json.Marshal` will do our work for us! (And, once we have syntax for it (#14), we'll be able to handle field-level omitempty basically for free.) More importantly, it will simplify custom marshalers (#38, forthcoming) significantly, since we do all that logic at the containing-struct level, but will need to apply it to arguments. It does require two breaking changes: 1. For folks implementing the `graphql.Client` API (rather than just calling `NewClient`): we now pass them variables as an `interface{}` rather than a `map[string]interface{}`. For most callers, including Khan/webapp, this is basically a one-line change to the signature of their `MakeRequest`, and it should be a lot more future-proof. 2. genqlient's handling of the `omitempty` option has changed to match that of `encoding/json`, in particular it now never considers structs "empty". The difference was never intentional (I just didn't realize that behavior of `encoding/json`); arguably our behavior was more useful but I think that's outweighed by the value of consistency with `encoding/json` as well as the simpler and more correct implementation (fixing #43 is actually quite nontrivial otherwise). Once we have custom unmarshaler support (#38), users will be able to map a zero value to JSON null if they wish, which is mostly if not entirely equivalent for GraphQL's purposes. Issue: https://github.com/Khan/genqlient/issues/38 Issue: https://github.com/Khan/genqlient/issues/43 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14) Pull Request URL: https://github.com/Khan/genqlient/pull/103 |
||
|
Ben KraftandGitHub
|
fcae8dd1d7 |
Add support for specifying type-names, and conflict-detection (#94)
## Summary: In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: https://github.com/Khan/genqlient/issues/60 Issue: https://github.com/Khan/genqlient/issues/12 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, jvoll, benjaminjkraft, aberkan, csilvers, dnerdy, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, jvoll 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: https://github.com/Khan/genqlient/pull/94 |