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

Take an interface for http.Client #33

Open
jlordiales opened this issue May 22, 2019 · 2 comments
Open

Take an interface for http.Client #33

jlordiales opened this issue May 22, 2019 · 2 comments

Comments

@jlordiales
Copy link

It would be nice if https://github.com/machinebox/graphql/blob/master/graphql.go#L48 could take a Doer interface, compatible with the std lib *http.Client so that clients can pass in whatever http client they want:

type Doer interface {
	Do(req *http.Request) (*http.Response, error)
}

This shouldn't break any clients using the std lib today.

I'd be happy to create a PR if you don't see any issue with the change.

@sunfmin
Copy link

sunfmin commented May 30, 2019

Just FYI, You can pass in a Transport to go HTTP client to do exactly the same. Like in this repo: https://github.com/sunfmin/handlertransport, I make the HTTP client doesn't request a remote address but go through a local HTTP handler.

	client := &http.Client{
	    Transport: handlertransport.New(http.HandlerFunc(hf)),
	}
	

@jlordiales
Copy link
Author

@sunfmin yeah, RoundTripper is nice for simple use cases but it has some important limitations/expectations for a general http client middleware:

  • RoundTrip should not modify the request, except for consuming and closing the Request's Body
  • RoundTrip should not attempt to handle higher-level protocol details such as redirects, authentication, or cookies

Taking a Doer interface instead of a *http.Client would still allow you to use the std lib client with a custom transport though.

Rarian added a commit to Rarian/graphql that referenced this issue May 20, 2020
This is a quick hack to replace http.Client with pester.Client, in lieu
of a better solution like a transport raised in machinebox#33.
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

No branches or pull requests

2 participants