Skip to content

Registrar/Resolver Redesign #1657

@rcdailey

Description

@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);
    }
}

Please upvote 👍 this issue if you are interested in it.

Metadata

Metadata

Assignees

No one assigned

    Projects

    Status

    Todo 🕑

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions