Skip to content

[Blazor][Wasm] Move HttpClient from default services to Program.Main #19119

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 20, 2020
Merged

[Blazor][Wasm] Move HttpClient from default services to Program.Main #19119

merged 2 commits into from
Feb 20, 2020

Conversation

Regenhardt
Copy link
Contributor

Summary of the changes (Less than 80 chars)

  • Removed HttpClient from default services
  • Added HttpClient to services from Program.Main method

Addresses #16929

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Feb 18, 2020
@dnfclas
Copy link

dnfclas commented Feb 18, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@Regenhardt
Copy link
Contributor Author

So turns out I used the wrong base branch to create a new one, brb.

@Regenhardt Regenhardt force-pushed the feature/blazor-wasm-no-default-httpclient branch from 74f980f to 2e0479e Compare February 18, 2020 19:49
@Regenhardt
Copy link
Contributor Author

Unfortunately I don't understand how to get to the artifacts from the failed tests.

ajaybhargavb
ajaybhargavb suggested changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example failed test,

OpenQA.Selenium.BrowserAssertFailedException : Xunit.Sdk.NotEmptyException: Assert.NotEmpty() Failure\r\n at Xunit.Assert.NotEmpty(IEnumerable collection) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\CollectionAsserts.cs:line 331\r\n at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass14_0.<Exists>b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 66\r\n at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass16_01.<WaitAssertCore>b__0(IWebDriver _) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 97\r\nScreen shot captured at 'F:\\workspace\\_work\\1\\s\\artifacts\\TestResults\\Release\\Microsoft.AspNetCore.Components.E2ETests\\7da499deaf6e42829c2360a57f50620a.png'\r\nEncountered browser errors\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: Unhandled exception rendering component:\"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: System.InvalidOperationException: Unable to resolve service for type 'System.Net.Http.HttpClient' while attempting to activate 'BasicTestApp.AuthTest.ServerAuthenticationStateProvider'.\"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateArgumentCallSites (System.Type serviceType, System.Type implementationType, Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain callSiteChain, System.Reflection.ParameterInfo[] parameters, System.Boolean throwIfCallSiteNotFound) \\u003C0x20ecda0 + 0x0009e> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite (Microsoft.Extensions.DependencyInjection.ServiceLookup.ResultCache lifetime, System.Type serviceType, System.Type implementationType, Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain callSiteChain) \\u003C0x1e4c258 + 0x000f0> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact (Microsoft.Extensions.DependencyInjection.ServiceDescriptor descriptor, System.Type serviceType, Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain callSiteChain, System.Int32 slot) \\u003C0x1e4b1d0 + 0x000f2> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact (System.Type serviceType, Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain callSiteChain) \\u003C0x1e4adc8 + 0x00034> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite (System.Type serviceType, Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteChain callSiteChain) \\u003C0x1e458b0 + 0x0005a> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory+\\u003C>c__DisplayClass7_0.\\u003CGetCallSite>b__0 (System.Type type) \\u003C0x1e453e8 + 0x00010> in \\u003Cfilename unknown>:0 \"\r\n[2020-02-18T20:01:59Z] [Severe] http://127.0.0.1:7484/subdir/_framework/blazor.webassembly.js 0:35547 \"WASM: at (wrapper delegate-invoke) System.Func2[System.Type,Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceCallSite].invoke_TResult_T(System.Type

Stacktrace:

at Microsoft.AspNetCore.E2ETesting.WaitAssert.WaitAssertCore[TResult](IWebDriver driver, Func`1 assertion, TimeSpan timeout) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 117
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.Exists(IWebDriver driver, By finder, TimeSpan timeout) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 61
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.Exists(IWebDriver driver, By finder) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 48
   at Microsoft.AspNetCore.Components.E2ETest.Tests.AuthTest.MountAndNavigateToAuthTest(String authLinkText) in /_/src/Components/test/E2ETest/Tests/AuthTest.cs:line 234
   at Microsoft.AspNetCore.Components.E2ETest.Tests.AuthTest.AuthorizeViewCases_RequireRole_NotAuthorized() in /_/src/Components/test/E2ETest/Tests/AuthTest.cs:line 118
----- Inner Stack Trace -----
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass14_0.<Exists>b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 66
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass16_0`1.<WaitAssertCore>b__0(IWebDriver _) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 97

@ajaybhargavb
Copy link
Contributor

@Regenhardt, thanks for the PR. It looks like some of our test apps also need this service injected. I suggest you run these tests locally to figure out which test apps needs this.

@pranavkm pranavkm added the feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly label Feb 19, 2020
@pranavkm pranavkm added this to the blazor-wasm-3.2-preview2 milestone Feb 19, 2020
@pranavkm
Copy link
Contributor

@Regenhardt you'll have to update the Blazor WASM based apps to add an HttpClient. For instance, https://github.com/dotnet/aspnetcore/blob/master/src/Components/test/testassets/BasicTestApp/Startup.cs#L16 would need one configured.

SteveSandersonMS
SteveSandersonMS reviewed Feb 19, 2020
View reviewed changes
...omponentsWebAssembly.ProjectTemplates/content/ComponentsWebAssembly-CSharp/Client/Program.cs Outdated
{
BaseAddress = new Uri(navigationManager.BaseUri)
};
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is going to be too much non-obvious code to add into Program.cs in the default template.

So as a recommendation, how about adding a new class in src\Components\WebAssembly\WebAssembly\src\Services like:

namespace Microsoft.Extensions.DependencyInjection
{
    public static class HttpClientServiceCollectionExtensions
    {
        public static void AddBaseAddressHttpClient(this IServiceCollection serviceCollection)
        {
            serviceCollection.AddSingleton(s =>
            {
                // Creating the URI helper needs to wait until the JS Runtime is initialized, so defer it.
                var navigationManager = s.GetRequiredService<NavigationManager>();
                return new HttpClient
                {
                    BaseAddress = new Uri(navigationManager.BaseUri)
                };
            });
        }
    }
}

Then in the project template, we just add services.AddBaseAddressHttpClient();.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. How about .AddDefaultHttpClient(); though?
The BaseAddress part would be confusing to me if I didn't know the code, since I just now learned that it means navigationManager.BaseUri in this context. Up until now, I would have wondered "So where does it get the BaseAddress for the HttpClient?".
Which is coincidentally literally what I've been wondering since I started working with Blazor, as I'm planning to use CORS and had to find out how to safely change the default BaseAddress.
Or put it in the description?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start with AddBaseAddressHttpClient. We have a few API reviews set up at which point we can sort out the naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to extension method and applied that at the required spots.

pranavkm
pranavkm reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

@Regenhardt Regenhardt force-pushed the feature/blazor-wasm-no-default-httpclient branch from 464b516 to b0cf257 Compare February 19, 2020 22:38
@Regenhardt
Copy link
Contributor Author

Now rebased as a single commit.

@Regenhardt
Copy link
Contributor Author

How's your policy on clean commits? Should I sqash the commits together, or do we keep it like this?

@SteveSandersonMS
Copy link
Member

@Regenhardt It will be squashed into one commit by GitHub when we merge it.

@SteveSandersonMS SteveSandersonMS self-requested a review February 20, 2020 12:24
@SteveSandersonMS
Copy link
Member

@ajaybhargavb This should be good now. If you're happy with it, can you remove your "requested changes" flag and squash-merge when ready? Thanks!

@SteveSandersonMS SteveSandersonMS mentioned this pull request Feb 20, 2020
Closed
ajaybhargavb
ajaybhargavb approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@ajaybhargavb ajaybhargavb merged commit 6a85855 into dotnet:blazor-wasm Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm pranavkm left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@ajaybhargavb ajaybhargavb ajaybhargavb approved these changes

Assignees

@ajaybhargavb ajaybhargavb

Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly
Projects
None yet
Milestone
blazor-wasm-3.2-preview2
Development

Successfully merging this pull request may close these issues.
5 participants
@Regenhardt @dnfclas @ajaybhargavb @pranavkm @SteveSandersonMS