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

Support injection of HttpClient instances #125

Closed
martincostello opened this issue Jul 17, 2018 · 8 comments
Closed

Support injection of HttpClient instances #125

martincostello opened this issue Jul 17, 2018 · 8 comments

Comments

@martincostello
Copy link
Contributor

The Connection class should be extensible enough to allow for interoperability with the HttpClientFactory in .NET Core 2.1.

This would allow users to use the out-of-the-box GraphQL client in their applications easily while applying cross-cutting concerns like retries, handler pooling, centralised configurability etc. without having to implement IConnection themselves to remove the HttpClient instance created privately per Connection instance.

@StanleyGoldman
Copy link
Collaborator

That is a very good suggestion.

@grokys
Copy link
Collaborator

grokys commented Jul 18, 2018

Would an additional ctor implemented like this be sufficient?

public Connection(HttpClient client, Uri uri, ICredentialStore credentialStore)
{
    Uri = uri;
    CredentialStore = credentialStore;
    HttpClient = client;
}

@martincostello
Copy link
Contributor Author

Maybe also one where the Uri isn’t required, given that I guess that exists just for Enterprise? The code would also need to add the UserAgent if not already present, or would you just leave that completely up to the integrator?

Happy to submit a PR if there’s a rough approach agreed.

@StanleyGoldman
Copy link
Collaborator

We are pretty interactive...
If you start me off with a unit test that shows us what you are thinking...
I can either take over or point you in the right directions to finish.

@martincostello
Copy link
Contributor Author

Cool, I'll give that a whirl when I get next a spare hour or so.

@martincostello
Copy link
Contributor Author

Have opened #131 with some very basic changes to Connection with a test showing the sort of things that can be achieved by supporting HttpClient injection.

@martincostello
Copy link
Contributor Author

I guess this is closed now #131 is merged? 😺

@StanleyGoldman
Copy link
Collaborator

Yup.

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

3 participants