Skip to content
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

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

csilvers
Copy link
Member

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:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

… 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 ./...
```
Copy link
Contributor

@dnerdy dnerdy left a 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.

@csilvers
Copy link
Member Author

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.

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a 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}}`
Copy link
Collaborator

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}}.

@csilvers csilvers merged commit 587f770 into main Nov 21, 2022
@csilvers csilvers deleted the expose-query branch November 21, 2022 16:35
@vikstrous2
Copy link
Contributor

vikstrous2 commented Nov 22, 2022

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

#241

@benjaminjkraft
Copy link
Collaborator

@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.)

michaeldwan added a commit to superfly/flyctl that referenced this pull request Jul 14, 2023
Updated genqlient library writes queries as constants now. See more: Khan/genqlient#238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export the query-over-the-wire as a constant
4 participants