Skip to content
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

[Feature Request] Adding the Commands Class to the service Container to enable access to it from other services via DI? #148

Open
Simonl9l opened this issue Sep 14, 2024 · 5 comments

Comments

@Simonl9l
Copy link

Simonl9l commented Sep 14, 2024

Is is correct that the following construct:

ConsoleApp.ServiceProvider = services.BuildServiceProvider();
var app = ConsoleApp.Create();
app.Add<MyCommands>();

and specifically app.Add<MyCommands>() is just a code signature the code generator runs on as the implementation of the resultant ConsoleApp.Add<T>() is empty:

public void Add<T>() { }

In the case that one is using DI, is it not unrealistic that a registered service might want to interact with the command class via an interface that could be injected into it?

Today is seem the code just new's up the Commands class (getting services from the DI container) and calls the requisite public function.

Could and alternative approach be to register the Command Class as say a Singleton (say), and any interfaces on it that one wants to leverage from other services.

services.AddSingleton<IMyService, MyService>();
services.AddSingleton<MyCommandClass>();
services.AddSingleton<IMyInterface>(provider => provider.GetRequiredService<MyCommandClass>());

And then I the generated use the ConsoleApp.ServiceProvider to instantiate the the Command Class (potentially updating these lines in the Emitter.cs - assuming the parser has the requisite information):

...
 var instance = ServiceProvider!.GetRequiredService<global::MyCommandClass>();
...

The DI container will the inject the required services as needed based the constructor, and invoke the Command Function as before.

Also a potential advantage on registering and getting the Command Class back from the DI Container, is that is will (from Net8.0) handle Async/Disposal in the container - not that the library has not already solved for that otherwise - in a non DI way.

@Simonl9l Simonl9l changed the title [Feature Request] Adding the Commands Class to the service Container to enable access to it fro other services via DI? [Feature Request] Adding the Commands Class to the service Container to enable access to it from other services via DI? Sep 15, 2024
@neuecc
Copy link
Member

neuecc commented Sep 17, 2024

Add<T> is probably necessary to pass type information to the Source Generator.
It may be possible to make a change that prioritizes the Service Provider in instantiation.

var instance = provider.GetService<MyCommands>() ?? new MyCommands(); /* when T is concrete class, fallback */

If this satisfies your needs, I will consider implementing it.

@Simonl9l
Copy link
Author

Hola @neuecc - been distracted, thanks for coming back on this!

So the assumption will be that both the concrete class as well as its interfaces (per my example above) that are registered in the DI container. This could work, but does have a slight smell in that other services "could" grab (DI) the entire command. If this is what you can do that that would be great!

Another approach dependent on the desired degree of coupling between the library and the DI container, could be:

That there be a number of Add<TC> , and say Add<TC, TI1>, Add<TC, TI1, TI2>... and the Command <TC> is new'd up as it is today and that the ConsoleApp owns the Service Container (or creates it, if it needed per the overrides), and each Add registers each of the interfaces <TI1> etc. Some of the .Net generics seem to support up to 8x variants, see Action for example.

The Services Container would be accessible as an additional property, similar to the ServiceProvider such that other services can be registered.

I then guess the Run /RunAsync would then build the container, and expose that on the ServiceProvider property.

As above - I'd be happy to work with the more basic approach.

At some point there might be a request to support different DI containers, sure sure how that would then play in either way.

@neuecc
Copy link
Member

neuecc commented Sep 19, 2024

Hmm, by the way, are you actually struggling with this in a real-world case?
These kinds of things often involve design trade-offs, so it's not advisable to unnecessarily add complexity just because it seems theoretically correct.

@Simonl9l
Copy link
Author

@neuecc

Hmm, by the way, are you actually struggling with this in a real-world case? These kinds of things often involve design trade-offs, so it's not advisable to unnecessarily add complexity just because it seems theoretically correct.

yes, I have a specific case for this. It's kinda of proprietary so it difficult to share the specifics, but do have a case when the Command launches other services that are DI'd, and there is status monitoring between then and the "state" of completeness of the cli command.

I'd suggest this is not an uncommon pattern?

I've a workaround that solves this for now by using an adapter service also registered in the DI container that implements IMyInterface the command injects this in and in turn registers it's IMyInterface interface with (this as IMyInterface) and the adapter delegates the interface calls though to the command. Whilst it works it an extra call on the stack, and it a tad kludgy?!?

If this satisfies your needs, I will consider implementing it.

Per you reply above what are you consideration timelines, and other change consideration per other issues - as would tie to a release?
Do you have a roadmap?
Is there something like a contribution I can do to help support that?

I'd also suggest this issue is also similarly valuable to the current use case - #142

@neuecc
Copy link
Member

neuecc commented Sep 24, 2024

To be honest, I don't understand more than half of what's being said.
As a result, I don't think any of the priorities are particularly high.

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

No branches or pull requests

2 participants