-
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
Add option for including operationName as a query parameter #298
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,9 +41,15 @@ type Client interface { | |
} | ||
|
||
type client struct { | ||
httpClient Doer | ||
endpoint string | ||
method string | ||
httpClient Doer | ||
endpoint string | ||
method string | ||
withOperationNameParam bool | ||
} | ||
|
||
// WithOperationNameParam allows operationName to be included as a query parameter. | ||
func WithOperationNameParam(c *client) { | ||
c.withOperationNameParam = true | ||
} | ||
|
||
// NewClient returns a [Client] which makes requests to the given endpoint, | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically a breaking change but probably one we can get away with |
||
return newClient(endpoint, httpClient, http.MethodPost, options...) | ||
} | ||
|
||
// NewClientUsingGet returns a [Client] which makes GET requests to the given | ||
|
@@ -83,11 +89,19 @@ func NewClientUsingGet(endpoint string, httpClient Doer) Client { | |
return newClient(endpoint, httpClient, http.MethodGet) | ||
} | ||
|
||
func newClient(endpoint string, httpClient Doer, method string) Client { | ||
func newClient(endpoint string, httpClient Doer, method string, options ...func(*client)) Client { | ||
if httpClient == nil || httpClient == (*http.Client)(nil) { | ||
httpClient = http.DefaultClient | ||
} | ||
return &client{httpClient, endpoint, method} | ||
c := &client{ | ||
httpClient: httpClient, | ||
endpoint: endpoint, | ||
method: method, | ||
} | ||
for _, opt := range options { | ||
opt(c) | ||
} | ||
return c | ||
} | ||
|
||
// Doer encapsulates the methods from [*http.Client] needed by [Client]. | ||
|
@@ -178,14 +192,31 @@ func (c *client) MakeRequest(ctx context.Context, req *Request, resp *Response) | |
} | ||
|
||
func (c *client) createPostRequest(req *Request) (*http.Request, error) { | ||
parsedURL, err := url.Parse(c.endpoint) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
queryParams := parsedURL.Query() | ||
queryUpdated := false | ||
|
||
if c.withOperationNameParam && req.OpName != "" { | ||
queryParams.Set("operationName", req.OpName) | ||
queryUpdated = true | ||
} | ||
|
||
if queryUpdated { | ||
parsedURL.RawQuery = queryParams.Encode() | ||
} | ||
|
||
body, err := json.Marshal(req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
httpReq, err := http.NewRequest( | ||
c.method, | ||
c.endpoint, | ||
parsedURL.String(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
bytes.NewReader(body)) | ||
if err != nil { | ||
return nil, err | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
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.
please add a named type for the options so they show up nicely in the documentation