-
Notifications
You must be signed in to change notification settings - Fork 112
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
Expose the graphql operation as a constant in the generated genqlient file #238
Conversation
… file. 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 ran: ``` env UPDATE_SNAPSHOTS=1 go test ./... go generate ./... ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me!
@@ -1,3 +1,5 @@ | |||
const {{.Name}}Operation = `{{$.Body}}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well put a doc-comment on this? Probably just saying it's the query executed by {{.Name}}
.
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.) |
Oh, maybe this is exactly the case you guys were talking about where we have a query like:
|
Oops! @benjaminjkraft how do you feel about naming it I'm not sure why folks haven't seen the same problem with |
@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 I merged the change for 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.) |
Updated genqlient library writes queries as constants now. See more: Khan/genqlient#238
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: