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

Non-static client factory #99

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

Conversation

mburumaxwell
Copy link

Microsoft advises that we should IHttpClientFactory to implement resilient HTTP requests (see docs). For this to happen, the HttpClient needs to be provided when creating instances of PostmarkClient or PostmarkAdminClient.

In this PR, the static client factory is replaced with an optional ISimpleHttpClient parameter. This works for scenarios where the injection of HttpClient instances is (and is not) preferred.

@mburumaxwell
Copy link
Author

@vladsandu any chance you can have a look?

@jchoca
Copy link
Contributor

jchoca commented Nov 11, 2021

Hey @mburumaxwell sorry for the delay -- we'll take a look. Vlad no longer works at Wildbit.

@jchoca
Copy link
Contributor

jchoca commented Nov 11, 2021

@mburumaxwell is there a specific problem the current interface/design is causing for you that you are trying to solve with this PR?

While it is true that HttpClient should be re-used, and it is also true that MS docs currently suggest creating clients from IHttpClientFactory, I'm not sure this change is worth the risk imposed, and the testing effort that would be required.

@mburumaxwell
Copy link
Author

@jchoca the intention is to inject the client using instances form IHttpClientFactory which brings a couple of benefits, including logging. I just thought it would be better to do a different PR for the dependency injection scenario.

@jchoca
Copy link
Contributor

jchoca commented Nov 11, 2021

What would that other PR consist of?

@Archanian
Copy link
Contributor

@mburumaxwell - ClientFactory is already a static property, you are free to change it if you'd like to provide a different factory method for creating the client.

@mburumaxwell
Copy link
Author

Unfortunately, that doesn't work well with a dependency injection.

It requires a lot of effort to setup get services.AddHttpClient<PostmarkClient>(...) working with it.

@Archanian
Copy link
Contributor

Fair enough, I understand your concern. However, conformance to a DI mechanism is not a strong enough reason to make this change.

@mburumaxwell
Copy link
Author

mburumaxwell commented Nov 14, 2021

@Archanian could you explain how you arrived at the decision for "not a strong enough reason"? For clarity purposes, what would break or not work as expected?

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.

3 participants