Khan / genqlient

a truly type-safe Go GraphQL client
MIT License
1.07k stars 107 forks source link

Expose the graphql operation as a constant in the generated genqlient file #238

Closed csilvers closed 1 year ago

csilvers commented 1 year ago

This allows client code to see the operation (query or mutation) exactly as genqlient sends it over the wire. This data was already available in the generated safelist.json file, but now it's easily available from Go code as well.

Fixes #236

I have:

csilvers commented 1 year ago

This looks good to me!

Note that the constant name could conflict with a separate operation with the same name, e.g. getViewer and getViewerOperation (which would have the operation string getViewerOperationOperation). Such a case seems very unlikely, though (and is definitely confusing), so I don't think we need to worry about it.

Yeah, the same is currently true of getViewerResponse as well. I agree we probably don't need to worry about it.

vikstrous2 commented 1 year ago

This is causing a bug in our generated code. The name of an interface is the same as the name of this constant. It seems like "Operation" is used as a suffix for some existing identifiers in some cases. Maybe you can use the suffix "OperationQuery" instead?

(Sorry, I don't know enough about genqlient to explain this well.)

vikstrous2 commented 1 year ago

Oh, maybe this is exactly the case you guys were talking about where we have a query like:

query GetX($operationID: String!) {
  operation(operationID: $operationID) {
    ... SomeOperationType
  }
}
csilvers commented 1 year ago

Oops! @benjaminjkraft how do you feel about naming it {{.Name}}_Operation? Ugly, but should be safe.

I'm not sure why folks haven't seen the same problem with {{.Name}}Response, but maybe that's safe for reasons unclear.

csilvers commented 1 year ago

https://github.com/Khan/genqlient/pull/241

benjaminjkraft commented 1 year ago

@vikstrous2 thanks for the report, it seems this case is more likely than we thought. I guess it makes sense when you point out that it can collide with a toplevel field (not just a similarly-named query), where operation is a totally reasonable name (probably way more common than response).

I merged the change for {{.Name}}_Operation to start with. (Thanks @csilvers for the quick fix!) I think that's fine if we don't come up with something better. Another option is to give it a prefix, e.g. RawOperationFor{{.Name}}, in hopes that that's a less likely collision, although then it doesn't sort as nicely. A weirder idea would be to put the query in a map or struct or something; then we just have to reserve one name globally. But that may have negative impacts if the operation-body can no longer be a const.

Ideally we'd do the same for Response, but that's probably not worth it at this point. If it comes up with Response we can make the pattern/suffix configurable. (Actually, another solution would be to put this behind an option which would also configure the pattern.)