Registrar/Resolver Redesign

[rcdailey]

I've never really been happy with the current type registrar/resolver model in Spectre.Console. Registration/resolution concerns are scattered across objects in the library with no clear line drawn between setup code and code with dependencies on services. This has been a complaint of mine going pretty far back.

The command interceptor logic introduced recently made the mutual exclusivity of the registrar/resolver design a little less painful, but I still often find myself in catch 22 scenarios simply because Spectre.Console steals away control over my own DI container from me.

I'd like to end this once and for all by completely rethinking the way Spectre.Console approaches DI. Instead of implementing its own nested facades, adopt a more modern approach. Something along the lines of this:

var services = new ServiceCollection();

services.AddSpectreConsole(config =>
{
    config.AddSetting("demo");
});

var provider = services.BuildServiceProvider();

var app = provider.GetRequiredService<ICommandApp>();
return app.Run(args);

There's certainly different approaches here. There's a fluent builder pattern that might be more appropriate here as well, although I will say that when I tried it, it didn't give me that separation between registration & resolution like this approach does.

I'll provide a more comprehensive implementation of this below. Instead of diving into the Spectre.Console code base and going crazy with refactoring, I wanted to start with a small demo to mimick some of the top level objects in the library and show the approach without much concern for implementation detail right off the bat.

This also gives us a chance to discuss the idea and see if the maintainers are interested in an overhaul in this area. I understand backward compatibility will be a big concern here. I honestly don't see a way to keep things backward compatible and still be able to overhaul the aspects of this that desperately need it, IMO.

Let me know how you folks feel about this. I'm happy to help drive the work, I just want to make sure it's not a PR bound to be rejected.

Thanks in advance.

NOTE: In the demo, I'm using the nuget package Microsoft.Extensions.DependencyInjection, but Spectre.Console will actually only use the abstractions (i.e. Microsoft.Extensions.DependencyInjection.Abstractions). I think going forward, the main spectre.console package should only be concerned with the IServiceProvider/IServiceCollection abstractions which allow users to adapt these just as they do today with the registrar/resolver facades. In the future we can add packages like Spectre.Console.DependencyInjection, where we might provide additional facilities like the AddSpectreConsole() method.

I'm open to opinion here but I'm just assuming you'd want to separate DI-specific implementations into their own packages because you might want to support other containers like Autofac separately without impacting the main library.

using System;
using System.Collections.Generic;
using Microsoft.Extensions.DependencyInjection;

namespace ConsoleApp1;

public interface IConfigurator
{
    void AddSetting(string value);
}

public class Configurator : IConfigurator
{
    private readonly List<string> _settings = [];

    public void AddSetting(string value)
    {
        _settings.Add(value);
    }
}

public interface ICommandApp
{
    int Run(IEnumerable<string> args);
}

internal class CommandApp : ICommandApp
{
    public readonly CommandExecutor _executor;
    private readonly IConfigurator _config;

    public CommandApp(IServiceProvider provider, IConfigurator config)
    {
        // CommandExecutor should probably be injected via constructor arg...
        _executor = provider.GetRequiredService<CommandExecutor>();
        _config = config;
    }

    public int Run(IEnumerable<string> args)
    {
        return _executor.Execute(_config, args);
    }
}

internal class CommandExecutor
{
    public int Execute(IConfigurator config, IEnumerable<string> args)
    {
        // do meaningful work
        return 0;
    }
}

public static class SpectreConsoleExtensions
{
    public static void AddSpectreConsole(
        this IServiceCollection services,
        Action<IConfigurator> configure)
    {
        services.AddTransient<ICommandApp, CommandApp>();
        services.AddTransient<CommandExecutor>();
        services.AddTransient<IConfigurator>(_ =>
        {
            var config = new Configurator();
            configure(config);

            // This is where the built-in (hidden) commands get added
            config.AddSetting("default stuff");

            return config;
        });
    }
}

public static class Program
{
    public static int Main(string[] args)
    {
        var services = new ServiceCollection();

        services.AddSpectreConsole(config =>
        {
            config.AddSetting("demo");
        });

        var provider = services.BuildServiceProvider();

        var app = provider.GetRequiredService<ICommandApp>();
        return app.Run(args);
    }
}

[patriksvensson]

@rcdailey I'm not against the idea per se, as long as there is no breaking change in the API. We're about to ship 1.0 of Spectre.Console and Spectre.Console.Cli, and we can't really introduce breaking changes to the API at this point.

[rcdailey]

Thanks @patriksvensson. Would the milestone still be 2.0 as opposed to a minor/feature release if I were able to maintain backward compatibility?

I won't say that backward compatibility is impossible, but it will certainly be challenging. Any advice or tips you can share in this area would be valuable. I'd also like to make sure you're OK with the general approach I've chosen.

I decided to introduce the DI abstractions instead of keeping your current interfaces for a few reasons:

• Most DI frameworks (that I've used) tend to already interop with the MS.DI abstractions.
• They're a bit more typesafe and functional
• Most developers will already know how to use them

I'll give this some more thought and share progress along the way; I definitely would like for this to be a collaborative effort. If you have a discord server or something we can chat on that'd also be great, if you wouldn't be bothered by me!

Thank you for the fast response.

[rcdailey]

@patriksvensson In addition to my previous reply, I have another question:

        // Register the arguments with the container.
        _registrar.RegisterInstance(typeof(CommandTreeParserResult), parsedResult);
        _registrar.RegisterInstance(typeof(IRemainingArguments), parsedResult.Remaining);

How important are these registrations in CommandExecutor.Execute()? I'm not really seeing anywhere they might be injected.

[nils-a]

@rcdailey this will force all users to use the "Microsoft DI" and restrict everyone else who likes to use some other DI container, right?

[rcdailey]

@rcdailey this will force all users to use the "Microsoft DI" and restrict everyone else who likes to use some other DI container, right?

No, that is something I'm deliberately trying to avoid, but I'd like to share my plan below for feedback and also to make sure things really will go as I hope.

• Main Spectre.Console Package

  Instead of using our own abstractions for DI concerns, such as ITypeRegistrar and ITypeResolver, consume dependencies directly in constructors. For example, in one instance, the CommandApp constructor would take a CommandExecutor (or abstraction of it) and then using the fluent builder pattern, remove construction concerns from the user (i.e. factory).

• New Spectre.Console.DependencyInjection Package

  This package would introduce extension methods for IServiceCollection (e.g. IServiceCollection.AddSpectreConsole()) that support transparently registering internal dependencies into the MS DI container, without users having to be aware of the ins & outs of how the registrations work and what types are needed.

The benefits I'm aiming for:

• Clearer separation of concerns (DI-related code won't be sprinkled across the Spectre.Console code base)
• More flexibility for users. Users have more control over what code is executed between the registration & configure steps of Spectre.Console and actual execution (e.g. RunAsync()) of the CLI application. The command interceptors are supposed to fill this gap today but they add a layer of complexity and are still not entirely flexible enough.
• Users no longer have to implement their own DI container adapters. The responsibility of DI container support is shifted to specific extension packages. I don't suspect these to really ever change, so I think the maintainers taking on this responsibility is a good balance for achieving higher usability.

I think achieving these goals would be much easier if we were willing to accept breaking changes. It might be possible to avoid it, but it will result in a significant amount of problem solving and effort that I do not personally believe to be worth it. My principle is always to avoid backward breaking changes, so this isn't an opinion I take lightly. However, if you all feel strongly about backward compatibility, I will do my best to find an iterative approach that allows for it.

I would likely take a two step approach:

• First PR would likely be an effort to refactor the existing implementation to shift existing registrations into the CommandApp.Configure() method, and greatly simplify the amount of setup work that is happening in the CommandExecutor class.
• Second PR would lay the foundation for passing parameters directly into constructors to make them DI-friendly, and also probably introduce the fluent builder system at this time.
• Third PR would probably be the MS DI-specific package. The reason I start with MS DI is because many other DI frameworks (e.g. Autofac) already have adapters for MS DI so I think starting here gets us the best bang for our buck, so to speak, in terms of DI compatibility across the board.

I hope my explanation is helpful. If my plan sounds too ambitious or just impossible let me know. Happy to adjust, after all, I need you all to be on board with this for PR approvals and I really don't want to waste effort if I can avoid it.

[rcdailey]

I noticed IPairDeconstructor is internal but CommandParameter supports a custom deconstructor via the PairDeconstructor property. However, because it's internal, no clients of the library can effectively implement their own deconstructor due to this logic (taken from CommandValueBinder):

        // Create deconstructor.
        var deconstructorType = parameter.PairDeconstructor?.Type ?? typeof(DefaultPairDeconstructor);
        if (!(resolver.Resolve(deconstructorType) is IPairDeconstructor deconstructor))
        {
            if (!(Activator.CreateInstance(deconstructorType) is IPairDeconstructor activatedDeconstructor))
            {
                throw new InvalidOperationException($"Could not create pair deconstructor.");
            }

            deconstructor = activatedDeconstructor;
        }

This leads me to believe that there are no active users for CommandParameter.PairDeconstructor since that type is required to implement IPairDeconstructor but they do not have access to it.

What is the intent here? Should the configurability of a "pair deconstructor" be removed from the public interface, or should the interface simply be made public?

[FrankRay78]

I've read none of the above @rcdailey, but when my plate is more clear (that shouldn't be too long away), I'm happy to work alongside you with this, if it's still of interest.

[rcdailey]

Thanks Frank. I'll try to renew interest later, but the slow turnaround makes any motivation or progress difficult. I understand everyone is spending their free time here, so I'm not upset and certainly not pointing fingers. But it is a reality, and unfortunately I barely have enough free time as it is working on my own projects.

I absolutely do want to maintain a helpful relationship so if you ever get some downtime let me know. I mentioned connecting on Discord earlier, if that's an option anyone is comfortable with, that would likely make communication much easier.

[phil-scott-78]

I would propose considering making this work tied closely with any AOT work (e.g. #1322, #1155, #955). Both are going to have some overlap on breaking changes and architecture rework, so both being in the same discussion would probably save some headaches in both directions

[FrankRay78]

@rcdailey There is a lull in maintainer activity/free time at the moment, except for myself, which is what you are seeing from the outside. Nothing untoward, just the reality of situation. I'm currently clearing down a few PR's and preparing the command line system for version 1 of Spectre.Console. But once that's out of the way, say early Dec, I can stick around and support any development you do, so you know it will be merged. That said, if it's too ambitious or breaking, then I'll become slowed down having to square it off with the other maintainers. But other things I can merge, and/or we could look at other issues if you feel like contributing there instead. Or simply acknowledging this is the situation currently, and waiting a bit. I think you're good in terms of 'maintain a helpful relationship', so don't let that cloud the decision how we proceed here.

[rcdailey]

Thanks again Frank. You've always been a pleasure to chat with on here so definitely will take advantage of your free time in December.

I think this is a challenging effort, especially if it means not breaking backward compatibility. I've mulled over a few different approaches but it involves a LOT of hacks and shims just to avoid breaking things. And the way the resolver is exposed to users in various structures and callbacks makes it even more challenging.

My inclination is to just break everything to get it the way we want, instead of tip toeing around compatibility. I understand that implies everything you mentioned, but maybe Phil's idea of bundling it with Native AOT work might make it more palatable (although I have no idea what that effort is yet, or if it's a good idea to build a bigger kitchen sink).

[kvpt]

I also noticed an issue with the current implementation that should be addressed if a new one is made.
Currently there is two main steps, the creation of the app (new CommandApp()) where the TypeRegistrar is passed and the app run (app.RunAsync).
From a DI perspective I expect that all dependencies registrations are made in the application building phase and not in the run phase, It is not currently the case.

I use myself SimpleInjector container which lock itself after the first dependency resolution.
So if I want to do something like :

• Build container
• Build app
• Resolve some dependencies
• Run app
  I can't, because the app try to register some dependencies in the run phase and can't because the container is locked.

[FrankRay78]

I want to understand the situation with Spectre.Console AOT trimming, hence my comment on 1508 here.

[FrankRay78]

From a DI perspective I expect that all dependencies registrations are made in the application building phase and not in the run phase, It is not currently the case.

nb. It's the following line that resolves the registrations, obviously deep in the command execution method:

spectre.console/src/Spectre.Console.Cli/Internal/CommandExecutor.cs

Line 60 in aa9e5c4

using (var resolver = new TypeResolverAdapter(_registrar.Build()))

If this was moved somewhere else @kvpt, how should it work?

For example.

new CommandApp(DI container)

(opportunity to continue modifying registrations here)

CommandApp.ResolveDependencies()

(container is locked, internally registered types locked also)

CommandApp.Run()

Is this what you had in mind?

[kvpt]

I think there is no need for a new method here, it only need to adhere on this two principles :

1. All the registrations need to be made in the build phase
   new CommandApp(DI container)

2. All the resolutions need to be done in the run phase
   CommandApp.Run()

Currently rule 2 seems ok, but rule 1 is not.

spectre.console/src/Spectre.Console.Cli/Internal/CommandExecutor.cs

Lines 13 to 28 in aa9e5c4

	public async Task<int> Execute(IConfiguration configuration, IEnumerable<string> args)
	{
	if (configuration == null)
	{
	throw new ArgumentNullException(nameof(configuration));
	}
	var arguments = args.ToSafeReadOnlyList();
	_registrar.RegisterInstance(typeof(IConfiguration), configuration);
	_registrar.RegisterLazy(typeof(IAnsiConsole), () => configuration.Settings.Console.GetConsole());
	// Create the command model.
	var model = CommandModelBuilder.Build(configuration);
	_registrar.RegisterInstance(typeof(CommandModel), model);
	_registrar.RegisterDependencies(model);

For exemple the executor register IConfiguration, IAnsiConsole and CommandModel.
Is it not possible to build and register them in build phase instead of doing it at the execution ?

[FrankRay78]

Yes, I see what you mean @kvpt.

I'm keen to look into whether a non-breaking refactor could separate out these different build/run concerns.

I'll update the thread once I've investigated further.

[wazzamatazz]

@rcdailey In case it's of any interest, I've got a project here that I use to integrate with Microsoft.Extensions.DependencyInjection (actually in most cases Microsoft.Extensions.Hosting) that you may or may not find useful.

[FrankRay78]

It's nearly December @rcdailey, my diary is emptying a little - how are you feeling about this issue?

[patriksvensson]

I'm going to take a look at this tonight and see if we can remove the TypeRegistrar/TypeResolver and instead take a dependency on the Microsoft.Extensions.DependencyInjection package.

[FrankRay78]

I'm going to take a look at this tonight and see if we can remove the TypeRegistrar/TypeResolver and instead take a dependency on the Microsoft.Extensions.DependencyInjection package.

@patriksvensson It would be worth keeping in mind @kvpt's comment above, when looking into this, see: #1657 (comment)

[rcdailey]

I'm going to take a look at this tonight and see if we can remove the TypeRegistrar/TypeResolver and instead take a dependency on the Microsoft.Extensions.DependencyInjection package.

Awesome news. I would see if you can instead get away with a dependency on just Microsoft.Extensions.DependencyInjection.Abstractions for the main package, this would allow users to adapt to their own DI system if they want. Then you can provide an extension package for actual concrete support for specific DI frameworks like Microsoft's.

[patriksvensson]

No, we still used a built-in DI container even if the user doesn't provide one, so would need to have an actual implementation as well.

[rcdailey]

I guess what I'm saying is that there's a strategy you could adopt where the Spectre.Console library itself uses constructor injection normally with no DI framework dependencies. Imagine using pure DI in this case. There would be nothing special about this, you're just using dependency inversion and factories.

Then via extension libraries, you can extend your configuration builder pattern to facilitate initialization of the library (i.e. provide its dependencies) using specific DI libraries, such as MS.DI, Autofac, etc. Users can also implement their own.

I'm not saying you should do this; it's just an approach. If you want to hard-code to support Microsoft.DI, then I completely understand. Other popular libraries like MassTransit do this, and it seems to work fine (except when you need to support older .NET frameworks).

Thanks again for acknowledging the issue and expressing support for it. If I can help at all please let me know.

[JKamsker]

I have solved a very similar issue in https://github.com/JKamsker/Spectre.Console.Extensions.Hosting-ng