-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Replace WebHostBuilder with HostBuilder pattern in MVC folder #62703
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: BrennanConroy <[email protected]>
…HostBuilder Co-authored-by: BrennanConroy <[email protected]>
…tBuilder to HostBuilder Co-authored-by: BrennanConroy <[email protected]>
Co-authored-by: BrennanConroy <[email protected]>
@@ -25,7 +25,6 @@ protected override void Initialize(TestContext context, MethodInfo methodInfo, o | |||
{ | |||
base.Initialize(context, methodInfo, testMethodArguments, testOutputHelper); | |||
Factory = new MvcTestFixture<RazorBuildWebSite.Startup>(LoggerFactory) | |||
.WithWebHostBuilder(b => b.UseStartup<RazorBuildWebSite.Startup>()) |
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.
Because the Startup type is the same as the one in the test fixture, we end up running both startups which both register a file provider and ends up messing with the cache-ability of razor views.
So removing this fixes the 2 quarantined tests below, and I'll mark #56553 as test-fixed.
We also might want to consider changing GenericWebHostBuilder
to detect the same startup type so it doesn't run it multiple times?
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.
Nice find! Handling this in GenericWebHostBuilder sounds like a good idea.
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.
Fixed in latest commit.
/// </summary> | ||
/// <param name="serviceProvider">The <see cref="IServiceProvider"/> from the bootstrapped application.</param> | ||
/// <returns></returns> | ||
protected virtual TestServer CreateServer(IServiceProvider serviceProvider) => new(serviceProvider); |
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.
We can remove this and consider it later if we want, as it introduces a new API.
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.
I'm inclined to keep it even though introducing a new API in an obsoletion pass feels funky since we don't expose any other nice ways for users to customize TestServer with a finalized DI container and we've gotten feedback in the past about how hard it is to plugin WAF APIs into different points in the minimal host's lifecycle (mostly before and after IHostBuilder.Build, but still).
@@ -1681,11 +1681,11 @@ public void Configure(IWebHostBuilder builder) | |||
// This check is required because MVC still uses the | |||
// IWebHostEnvironment instance before the container is baked | |||
#pragma warning disable CS0618 // Type or member is obsolete | |||
var heDescriptor = services.SingleOrDefault(s => s.ServiceType == typeof(IHostingEnvironment)); | |||
var heDescriptor = services.FirstOrDefault(s => s.ServiceType == typeof(IHostingEnvironment)); |
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.
Maybe this should be LastOrDefault
?
|
||
_builder.ConfigureServices((context, services) => | ||
{ | ||
// Run this delegate if the startup type matches | ||
if (object.ReferenceEquals(_startupObject, startupType)) | ||
if (_builder.Properties.TryGetValue(_startupConfigName, out var startupObject) && |
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.
Bug introduced by #24144 that allowed multiple startups to run if they spanned different IWebHostBuilder
instances. See test MultipleConfigureWebHostCallsWithUseStartupLastWins
below for example.
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.
Crazy
services.TryAddTransient<HtmlEncoder, HtmlTestEncoder>(); | ||
services.TryAddTransient<JavaScriptEncoder, JavaScriptTestEncoder>(); | ||
services.TryAddTransient<UrlEncoder, UrlTestEncoder>(); | ||
services.AddTransient<HtmlEncoder, HtmlTestEncoder>(); |
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.
Is there a reason for this change?
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.
Oh right, forgot about this one.
So the two hosts handle ordering differently.
var host = new WebHostBuilder()
.UseStartup<Startup>()
.ConfigureServices(services =>
{
services.TryAddSingleton<Bar>(new Bar() { Value = 10 });
})
.Build();
var host2 = new HostBuilder()
.ConfigureWebHost(webhostbuilder =>
webhostbuilder
.UseKestrel()
.UseStartup<Startup>()
)
.ConfigureWebHost(webhostbuilder =>
{
webhostbuilder.ConfigureServices(services =>
{
services.TryAddSingleton<Bar>(new Bar() { Value = 10 });
});
}).Build();
Console.WriteLine(host.Services.GetRequiredService<Bar>().Value);
Console.WriteLine(host2.Services.GetRequiredService<Bar>().Value);
public class Startup
{
public void Configure(IApplicationBuilder app)
{
}
public void ConfigureServices(IServiceCollection services)
{
services.TryAddSingleton<Bar>(new Bar() { Value = 30 });
}
}
public class Bar
{
public int Value { get; set; }
}
This prints:
10
30
It's too late to change eithers order, but HostBuilder
does feel a bit more natural since it does things more in order of how the code is ordered.
Summary
This PR replaces all usages of
WebHostBuilder
with the modernHostBuilder
andConfigureWebHost
pattern across the MVC folder, updating 24 files to follow current ASP.NET Core hosting best practices.Part of #20964
Changes Made
Pattern Transformation:
Files Updated (24 total):
Key Changes Applied
✅ Added using statements:
using Microsoft.Extensions.Hosting;
✅ Updated variable declarations:
var host
→using var host
✅ Renamed methods:
CreateWebHostBuilder()
→CreateHost()
✅ Changed return types:
IWebHostBuilder
→IHostBuilder
✅ Wrapped configuration:
new WebHostBuilder()
→new HostBuilder().ConfigureWebHost(...)
✅ Preserved all configurations: All original
.UseKestrel()
,.UseStartup<>()
,.UseConfiguration()
,.UseStaticWebAssets()
, etc. methods maintainedTesting
ConfigureWebHost
lambda.UseStaticWebAssets()
in TagHelpersWebSite correctly handledVerification
new WebHostBuilder()
usages in MVC foldernew HostBuilder()
patternThis pull request was created as a result of the following prompt from Copilot chat.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.