dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.37k stars 378 forks source link

Add UseHost for WebApplicationBuilder (or maybe IHostApplicationBuilder) #2268

Open djeikyb opened 1 year ago

djeikyb commented 1 year ago

My team maintains a bunch of http apis using asp core. We've added a cli interface to each app because tho starting kestrel is the main case, there are several others, for example

Some are only invoked when running on a local dev machine, others are used in deploy pipelines.

The included System.CommandLine.Hosting.HostingExtensions has a UseHost for an IHostBuilder, but the host build object I have is an WebApplicationBuilder. Building the entire host is somewhat expensive, to be sure. But it's useful enough to be worth it, and the things we're running need most of the app anyway.

So I copied the UseHost function and swapped in the WebApplicationBuilder type. Mostly it's the same, except that the host is neither stopped nor started. I want the command handlers to decide whether or not to start the host! My RunWebApi handler does start the host, but my SelfCheck handler only needs the built host's ServicesCollection.

Does it make sense to add this extension method to the library? As I wrote this out, I realized that maybe IHostApplicationBuilder is a better type than WebApplicationBuilder, but the idea stands.

using System.CommandLine.Builder;
using System.CommandLine.Invocation;

namespace System.CommandLine.Hosting;

public static class HostingExtensions
{
    private const string ConfigurationDirectiveName = "config";
    public static CommandLineBuilder UseHost(this CommandLineBuilder builder,
        Func<string[], WebApplicationBuilder> hostBuilderFactory,
        Action<WebApplicationBuilder>? configureHost = null) =>
        builder.AddMiddleware(async (invocation, next) =>
        {
            var argsRemaining = invocation.ParseResult.UnparsedTokens.ToArray();
            var hostBuilder = hostBuilderFactory.Invoke(argsRemaining);
            hostBuilder.Host.Properties[typeof(InvocationContext)] = invocation;

            hostBuilder.Host.ConfigureHostConfiguration(config =>
            {
                config.AddCommandLineDirectives(invocation.ParseResult, ConfigurationDirectiveName);
            });
            hostBuilder.Host.ConfigureServices(services =>
            {
                services.AddSingleton(invocation);
                services.AddSingleton(invocation.BindingContext);
                services.AddSingleton(invocation.Console);
                services.AddTransient(_ => invocation.InvocationResult);
                services.AddTransient(_ => invocation.ParseResult);
            });
            hostBuilder.Host.UseInvocationLifetime(invocation);
            configureHost?.Invoke(hostBuilder);

            using var host = hostBuilder.Build();

            invocation.BindingContext.AddService(typeof(WebApplication), _ => host);

            // await host.StartAsync(); // no: let cli handlers decide if they want to start the host

            await next(invocation);

            // await host.StopAsync(); // don't stop what wasn't started
        });
KalleOlaviNiemitalo commented 1 year ago

IHostApplicationBuilder only exists starting from .NET 8.0 preview 6 (https://github.com/dotnet/runtime/pull/86974, https://github.com/dotnet/aspnetcore/pull/48775) so this would have to be in #if NET8_0_OR_GREATER. There is some precedent for public APIs varying by target framework, as the System.CommandLine.Hosting.InvocationLifetime constructor takes an IHostingEnvironment parameter on .NET Standard 2.0 but IHostEnvironment on .NET Standard 2.1.

The HostingExtensions implementation is quite different on the main branch, because the middleware APIs have been replaced with CliAction.

djeikyb commented 1 year ago

Ah ok yeah, then the sample here definitely isn't useful, except maybe to one or two other folks running the preview of this lib with asp. The sentiment stands, tho I haven't reviewed the main branch just yet to see if this issue still makes sense beyond the latest preview release.

WeihanLi commented 1 year ago

Maybe we could add a new extension like UseWebHost or something else for WebApplicationBuilder, use UseHost for the generic host

fredrikhr commented 1 month ago

I have started work on PR #2450 where I propose new overloads to UseHost that accept HostApplicationBuilder.