JasperFx / oakton

Parsing and Utilities for Command Line Tools in .Net
http://jasperfx.github.io/oakton
Apache License 2.0
308 stars 41 forks source link

Add DI Hosted commands #93

Closed tgardner closed 2 months ago

tgardner commented 3 months ago

Fixes #92

jeremydmiller commented 2 months ago

@tgardner Shoot, I took this in too quickly. It's going to take some changes for disposal at a minimum.

This:

internal class DependencyInjectionCommandCreator : ICommandCreator
{
    private readonly IServiceProvider _serviceProvider;
    public DependencyInjectionCommandCreator(IServiceProvider serviceProvider)
    {
        _serviceProvider = serviceProvider;
    }

    public IOaktonCommand CreateCommand(Type commandType)
    {
// Right here, you need to do something to ensure that the scope is disposed to clean up stuff
        var scope = _serviceProvider.CreateScope();
        return (IOaktonCommand)scope.ServiceProvider.GetRequiredService(commandType);
    }

    public object CreateModel(Type modelType)
    {
        return Activator.CreateInstance(modelType)!;
    }
}

I'll build off what you did here as a starting point though.

tgardner commented 2 months ago

@jeremydmiller - I was under the belief that the scope would need to live as long as the command that it created did. Disposing the scope, will in turn dispose any objects created, resulting in an exception when trying to use inside the command.

I made some assumptions here, possibly wrong. That the scope would be disposed of once the command executes and application finishes.

I'm seeing a couple of options here:

  1. Currently it's creating a scope per command. This was to ensure that each command isn't populated with any shared scoped resource. We'd need a mechanism to tie the disposal of the scope to the disposal of the command.
  2. Create a single scope for the command creator. Move the scope creation to the constructor and dispose of it along with the CommandCreator. I don't like this as much, as in the case of DbContexts etc, you'll end up with potentially change tracked entities already present in command initialization.

I'm happy to help if you can provide any form of guideance.