Skip to content

Move to Minimal Hosting Model in a backwards compatible way #14656

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

Merged
merged 13 commits into from
Aug 21, 2023

Conversation

bergmania
Copy link
Member

@bergmania bergmania commented Aug 9, 2023

Description

This PR introduces the minimal hosting model.

It have been made in a backward compatible way. It throws friendly error messages, if the await app.BootUmbracoAsync(); have not been called early enough, when using minimal hosting model.

Tests

  • Smoke test that everything works as expected.
  • Test that it works with the old Program.cs and Startup.cs

@bergmania bergmania marked this pull request as ready for review August 10, 2023 08:57
@bergmania bergmania changed the title Move to Minimal Hosting Model Move to Minimal Hosting Model in a backwards compatible way Aug 10, 2023
Copy link
Contributor

@ronaldbarendse ronaldbarendse left a comment

Choose a reason for hiding this comment

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

@bergmania I've added some comments to give additional context and ensured UIseStaticWebAssets() is not called when using production runtime mode.

I've tested this by adding breakpoints to CoreRuntime.StartAsync() and CoreRuntime.StopAsync() and can verify they are only called once when using the Minimal Hosting model and also after copying the Program/Startup files from v12 👍🏻

@@ -152,12 +152,6 @@ private async Task StartAsync(CancellationToken cancellationToken, bool isRestar
// Store token, so we can re-use this during restart
_cancellationToken = cancellationToken;

// Just in-case HostBuilder.ConfigureUmbracoDefaults() isn't used (e.g. upgrade from 9 and ignored advice).
if (StaticServiceProvider.Instance == null!)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now set when BootUmbracoAsync() is called or when using the Program/Startup approach when the decorated IHostBuilder (returned by ConfigureUmbracoDefaults()) is built, and validated when calling UseUmbraco(), so that's why this fallback can be safely removed.

Comment on lines +34 to +38
if (addRuntimeHostedService)
{
// Add the Umbraco IRuntime as hosted service
builder.ConfigureServices(services => services.AddHostedService(factory => factory.GetRequiredService<IRuntime>()));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the Umbraco IRuntime still inherits from IHostedService, it's only added as hosted service when using the Program/Startup approach. This ensures the StartAsync() and StopAsync() methods are called only once and at the right time, regardless of whether the Minimal Hosting model is used or not.

@@ -15,6 +17,9 @@ public static class HostBuilderExtensions
/// Configures an existing <see cref="IHostBuilder" /> with defaults for an Umbraco application.
/// </summary>
public static IHostBuilder ConfigureUmbracoDefaults(this IHostBuilder builder)
=> builder.ConfigureUmbracoDefaults(true);

internal static IHostBuilder ConfigureUmbracoDefaults(this IHostBuilder builder, bool addRuntimeHostedService)
{
#if DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this conditional code, since it's only used when running a debug build from the source code. Released assemblies won't have this code, so debug builds of sites using the NuGet packages also won't have this optional appsettings JSON file added anyway.

We could add it to the Program.cs file again, although it will then be shipped in the default template:

#if DEBUG
builder.Configuration.AddJsonFile("appsettings.Local.json", optional: true, reloadOnChange: true);
#endif

Comment on lines +27 to +32
// Do not enable static web assets on production environments,
// because the files are already copied to the publish output folder.
if (builder.Configuration.GetRuntimeMode() != RuntimeMode.Production)
{
builder.WebHost.UseStaticWebAssets();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The static web assets should only be enabled when developing locally, since it will configure the WebRootFileProvider to get referenced files from the shared NuGet cache folder. Production environments should only serve files from the project (for security) and because the static web assets (from Razor Class Libraries) are already automatically copied to the publish output, this also removes some overhead in file lookups.

Copy link
Contributor

@ronaldbarendse ronaldbarendse Aug 17, 2023

Choose a reason for hiding this comment

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

Although I don't expect a lot of users will be running a non-Development environment using the Production runtime mode from source (non-published output), but if they do, the files from RCLs won't be available anymore and return 404s now.

This can be easily fixed by manually calling builder.WebHost.UseStaticWebAssets(); yourself. We could add this to the documentation, but I guess it's really an edge-case and could potentially result in users adding this without knowing/fully understanding the implications and not removing/disabling it on production again...

Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

Looks good, and tests good.

I've both tried upgrading an "old format" version to this branch and creating a new one, and both works as intended 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants