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

Add option for including operationName as a query parameter #298

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cgetzen
Copy link

@cgetzen cgetzen commented Dec 20, 2023

This introduces the ability to include operationName as a query parameter for the default client.

There are a few patterns for options that are described here. The existing NewClientUsingGet follows the "Declare a new constructor for each configuration option" pattern. As additional options get added, the "Functional Options Pattern" may be more scalable.

Use-case: This allows requests to be routed based on their operation.

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

@cgetzen
Copy link
Author

cgetzen commented Dec 20, 2023

Hi @StevenACoffman

The lint failures seem unrelated to this PR. I am also unable to trigger these errors locally with:

docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.52.2 golangci-lint run -v

Do you have any guidance on what could be going wrong? Thanks!

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.

Thanks for the PR! The code looks fine modulo a few comments, below.

But I do want to check: does this match any standard conventions? I don't see any specific problems here but I want to be a bit careful of going down the road of adding many options to the client. (One can always wrap the Doer (specifically, the http.Transport of the http.Client) instead -- indeed at Khan when we wrote genqlient we did as much to support something similar but with a slightly different convention. For NewClientUsingGet that's substantially harder but here it would be basically the same code.)

@@ -58,8 +64,8 @@ type client struct {
// example.
//
// [example/main.go]: https://github.com/Khan/genqlient/blob/main/example/main.go#L12-L20
func NewClient(endpoint string, httpClient Doer) Client {
return newClient(endpoint, httpClient, http.MethodPost)
func NewClient(endpoint string, httpClient Doer, options ...func(*client)) Client {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically a breaking change but probably one we can get away with

body, err := json.Marshal(req)
if err != nil {
return nil, err
}

httpReq, err := http.NewRequest(
c.method,
c.endpoint,
parsedURL.String(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is parse/unparse guaranteed to roundtrip a valid URL without changes? Unless we're sure, it's probably a tad safer if we change queryUpdated to urlUpdated and use the URL exactly as specified if we don't need to change it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add at least a quick test (either unit or integration, not sure which will be easier to wire up) that the option actually does something!

}

// WithOperationNameParam allows operationName to be included as a query parameter.
func WithOperationNameParam(c *client) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a named type for the options so they show up nicely in the documentation

@benjaminjkraft
Copy link
Collaborator

Ah, and I'm not sure what's up with the lint. I agree it looks off, but I don't have time to figure out why right now. Perhaps we should try updating the linter -- feel free to give that a go in a separate PR if you like!

@benjaminjkraft
Copy link
Collaborator

Lint is fixed in #311 so once you merge main that part should be good!

@StevenACoffman StevenACoffman removed their request for review May 28, 2024 11:24
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.

2 participants