-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 AddHttpClientDefaults #87953
Add AddHttpClientDefaults #87953
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #87914 The The method:
|
This feels pretty bad. |
@noahfalk @joperezr @geeknoid @davidfowl The The goal is to avoid having two methods:
For example, methods in dotnet/extensions below will no longer be needed: It will be possible to do: // Add HttpClientLogging to all clients.
services.AddHttpClientDefaults().AddHttpClientLogging();
// Is configured with HttpClientLogging from the defaults.
services.AddHttpClient(); |
I've also been thinking about an API like this and was hoping we could add an overload to AddHttpClient so it wasnt 2 different calls. services.AddHttpClient(IHttpClientBuilder clientBuilder =>
{
clientBuilder.AddHttpMessageHandler<MyAuthHandler>();
}); |
It's not the best, but it works well. It hits these important requirements:
The extra logic is only applied to |
@dpk83 FYI |
....Http/tests/Microsoft.Extensions.Http.Tests/DependencyInjection/AddHttpClientDefaultsTest.cs
Outdated
Show resolved
Hide resolved
Let's please agree on the API via discussion on the issue and API review before opening PRs. I spoke with @CarnaViire at length about this and other related APIs yesterday and I know she was going to be following up with you. |
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.
LGTM for @JamesNK 's changes
@dotnet/ncl can someone pls review as well? Since I've taken over the PR. |
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.
@CarnaViire changes LGTM.
This change appears to be breaking flow into aspnetcore in dotnet/aspnetcore#49433 |
@CarnaViire can you please take a look? |
I've pushed the fix to the branch dotnet/aspnetcore#49433 |
So we have a problem here. I tried to use ConfigureHttpClientDefaults and noticed it isn't an extension method in the reference assembly. |
Can you elaborate? This change seems to suggest that the reference assembly was edited and that the extension was properly added: https://github.com/dotnet/runtime/pull/87953/files#diff-06e422c9663ed696b723dc56832cbdb53b9f9c3e6d0a98e52a1cb081b7c9dc7aR50 |
Oh, completely missed it! Good catch @davidfowl and thanks for the quick fix 😃 |
Fixes #87914
The
AddHttpClientDefaults
method supports adding configuration to all createdHttpClients
.The method:
Microsoft.Extensions.Configuration
automatically applies configuration with a null name to all named configuration.IServiceCollection
. This is to make it so the order ofAddHttpClientDefaults
andAddHttpClient
doesn't matter. Default config is always applied first, then named config is applied after. This is done by wrapping theIServiceCollection
in an implementation that modifies the order thatIConfigureOptions<HttpClientFactoryOptions>
values are added.