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

Add support for dependency injection in Excel-DNA #20

Closed
augustoproiete opened this issue Aug 29, 2015 · 16 comments
Closed

Add support for dependency injection in Excel-DNA #20

augustoproiete opened this issue Aug 29, 2015 · 16 comments

Comments

@augustoproiete
Copy link
Member

I'd like to be able to setup my IoC container via an entry-point of the add-in (my understanding is that the AutoOpen of a class implementing IExcelAddIn will be the very first thing to run when Excel loads the add-in)

public class AddIn : IExcelAddIn
{
    public void AutoOpen()
    {
        // Setup my IoC container
        myContainer.Register<AwesomeService>.As<IMyService>();
        myContainer.Register<GreatLogger>.As<ILog>();
    }
}

And then be able to have dependency resolution on any of my Ribbons in the add-in, ideally via constructor injection (*).

[ComVisible(true)]
public class ModelingRibbon : ExcelRibbon
{
    public ModelingRibbon(IMyService service, ILog logger)
    {
        // ...
    }
}

(*) Not sure if constructor injection would not be possible... Looks like the Ribbon instance is constructed before the AutoOpen runs, which would invalidate the idea of setting up the container in the AutoOpen.


Anyway, goal is to have an entry point that allows the opportunity to setup an IoC container, and have dependency resolution across the different instances created by the framework.

@govert
Copy link
Member

govert commented Aug 29, 2015

Yes - AutoOpen should be a good place for you to do such initialization. I'm not sure how hard it would be to allow special constructors for your ribbon, but it will probably be easier to resolve the dependencies internally, and not through the constructor.

I don't want to add an IoC dependency into Excel-DNA, but I don't know of any problem in having that inside your add-in.

@augustoproiete
Copy link
Member Author

Thanks @govert.

Indeed, not sure how complicated would be to support injection via the constructor of the Ribbon (I have yet to see how (and who) instantiates it).

The issue I normally have with the Ribbon, is that there's not a single point of entry that I can inject all my dependencies, as different methods are called as part of the initialization.

e.g.

[ComVisible(true)]
public class MyCustomRibbon : ExcelRibbon
{
    public override string GetCustomUI(string ribbonId)
    {
        // Here I need several dependencies injected, that would have been registered in AutoOpen
        // e.g. logger, config settings, utility that reads (or builds) the ribbon XML
    }

    public void OnLoad(IRibbonUI ribbon)
    {
        // Here I also need several dependencies injected
        // e.g. logger, config settings, etc.

        // I also need to register this ribbon back to the IoC container, so that
        // the ViewModel can resolve it and interact with it (e.g. invalidate it)
    }
}

For this to be possible, Excel DNA would have to - at last - provide me with hooks so that I can plug-in my IoC container (whatever that is).

Would you be open to a PR (or set of PRs) that enables something like this?

@govert
Copy link
Member

govert commented Aug 29, 2015

I don't understand why you'd need any hooks from Excel-DNA for this scenario. Whether it's in Excel-DNA or in your own add-in library, there must me some global mechanism that holds together the IoC container or dependency injection root. I can't see any reason why this has to involve the ExcelDna.Integration at all.

So, to be clear, I would not like to support any IoC infrastructure inside Excel-DNA in the near term. I'm happy with any helpers or examples that should how this might be done inside the add-in library.

@govert
Copy link
Member

govert commented Aug 29, 2015

ExcelRibbon derives from ExcelComAddIn, and thus shares the lifetime and overridable methods from ExcelComAddIn (from the IDTExtensibility2 interface). In particular, OnConnection happens early, though I'm not sure whether it happens before GetCustomUI() But I'd expect the sequence to be deterministic, though it's actually up to Excel. No Ribbon callback method (including OnLoad) will ever be called before GetCustomUI.

@augustoproiete
Copy link
Member Author

augustoproiete commented Aug 29, 2015

@govert:

"I don't understand why you'd need any hooks from Excel-DNA for this scenario"

The reason is simple: From my add-in, I cannot control the instantiation of my IExcelAddIn implementation (or my IRibbon implementation), therefore I cannot resolve any dependencies that it might have - I don't have a chance to do it.

e.g.

public class AddIn : IExcelAddIn
{
    public void AutoOpen(IConfigurationSettings settings, IUserProfileService userProfileSvc)
    {
        var outputFolder = settings.WriteToFolder;
        // ...

        var profile = userProfileSvc.Get(Environment.CurrentUser);
        // ...
    }
}

I need to have a chance to tell Excel-DNA how to resolve IConfigurationSettings and IUserProfileService, because Excel-DNA is responsible for creating the instance of AddIn.

So at the very least, I would need Excel-DNA to give me control on how to instantiate the AddIn class and the ExcelRibbon class - instead of doing it on its own after searching for classes that implement IExcelAddIn, etc. or (ideally) Excel-DNA would provide me with a hook to plug-in my own dependency resolver, and it would defer to this dependency resolver when it cannot find an implementation for something.

@govert
Copy link
Member

govert commented Aug 29, 2015

I don't understand. Surely you can have:

public class MyAddInImplementation
{
    public MyAddInImplementation(IWhatever whatever) {}
}

public ExcelDnaAddIn : IExcelAddIn
{
    public void AutoOpen()
    {    
        var whatever = .... // Set up anything you want so that resolving stuff will work
        var myAddIn = new MyAddInImplementation(whatever);
    }
}

This kind of code will either be in ExcelDna.Integration or in the add-in library itself. For now it should go into the add-in.

@augustoproiete
Copy link
Member Author

@govert: Sure, that's pretty much what I have to do today for every single add-in that I support (more than 10, and growing) which is boring and boilerplate code.

But this is not the biggest issue, really.

The issue is with everything else that happens after the AutoOpen is executed, such as Ribbons, that require dependencies that have been registered in the AutoOpen, but now will not resolve as part of any dependency resolution mechanism - therefore forcing the developer to keep global static variables with the container (and other stuff) and then being forced to use the service locator pattern to resolve things.

Anyway, this goes hand-in-hand with issue #21, so will close it as well, and hopefully we can revisit it over time.

Thanks

@AlonAm
Copy link

AlonAm commented Jan 22, 2016

I'm having the same issues and I think your requirement is justified and indeed needed.
Such support would improve excel-dna significantly.

@AlonAm
Copy link

AlonAm commented Jan 22, 2016

Currently we're forced to use service-locator which is bad and inelegant.

I was thinking to fork excel-dna and enable hooking my container in a similar way like in Nancyfx (the web framework)

@govert
Copy link
Member

govert commented Jan 22, 2016

Can you explain the problem with setting up your mechanism in the IExcelAddIn.AutoOpen() and then doing the bootstrapping yourself (as in my earlier reply)?

Of course you can make a custom build of Excel-DNA with your own dependency resolution mechanism built in, but it seems to me that the mechanism might just as well be set up in your add-in code.

@robodude666
Copy link

robodude666 commented Oct 31, 2016

@augustoproiete Is the real issue that ExcelDNA will create an instance of any ExcelRibbon type on your behalf?

It seems for pre-2007 Excel you must manually create an instance of your ExcelComAddIn type and manually call ExcelComAddInHelper.LoadComAddIn to load your AddIn. That should allow the following scheme with IoC to work:

  • Use IExcelAddIn.AutoOpen to create your Container.
  • Register all of your types.
  • Ask the IoC Container for an instance of your ExcelComAddIn type.
  • Pass your new object to LoadComAddIn.

The above flow is broken for Excel 2007+ when ExcelDNA takes ownership of creating the ExcelComAddIn instance (in AssemblyLoader.GetExcelAddIns if I understand the code correctly).

If the above is true, would it not be possible to implement our own version of ExcelRibbon by creating our own IRibbonExtensibility definition and not letting ExcelDNA detect our AddIn as being a Ribbon AddIn?

Looking through the code I don't see any other uses of ExcelAddInInfo.IsCustomUI or ExcelAddInInfo.Instance that would be broken.

@govert Do you recall the reason for the different handling of Ribbon-based vs non-Ribbon-based Excel ComAddIns? It seems odd to require the developer to only worry about loading the AddIn for pre-2007.

-robodude666

@augustoproiete
Copy link
Member Author

@robodude666

... Is the real issue that ExcelDNA will create an instance of any ExcelRibbon type on your behalf?

That is one of the issues, yes.

But not the only one. Excel Functions, for example, can use services that ultimately should be resolved by the IoC container. There's no access to any Ribbon instance from AutoOpen, which means we don't have a way to register each of the Ribbon's instance with the container (unless you hack all the ribbons constructors to store themselves in an static collection that can be picked up by AutoOpen, which is quite ugly). The IRibbonUI (IIRC) is not the same as the instance of the ribbon itself, so more hacks required on GetCustomUI to register the instance with the container - however, that's too late and the container must have already been built as services may be required in GetCustomUI to build (or load) the XML, etc. The whole lifecycle is kind of messy.

... would it not be possible to implement our own version of ExcelRibbon by creating our own IRibbonExtensibility definition and not letting ExcelDNA detect our AddIn as being a Ribbon AddIn

Maybe. It's been a while since I last looked at the internals of ExcelDna, so don't remember much, but based on your comments I think it is def. worth a shot, so at least the Ribbon side could have a better approach to DI.

@robodude666
Copy link

robodude666 commented Nov 1, 2016

Forgot about the Excel Functions, as that's a valid point. Two possible solutions come to mind:

  1. It may be possible to use Registration as an example on how to create a custom IoC-wrapped ExcelFunction?

  2. Store the IoC container in a static class and access it from the ExcelFunction to get the instance for dependencies. To make it less dirty:

[ExcelFunction]
public static void MyFunction()
{
    var instance = Container.ResolveOrUseLastInstance<MyFunctionImpl>();
    instance.Execute();
}

The above allows you to still take use of constructors and test MyFunctionImpl directly. Technically this may not be any different than a Service Locator (anti-)pattern, but quite frankly is justified in this case given that ExcelDNA imposes the use of Static Methods.

  1. Bonus: Does C# have something similar to Python's functools.partial ? If so, I believe ExcelDNA allows for ExcelFunctions to be registered dynamically at the runtime. If so (again), it would be possible to create a function that has some of the arguments already defined via the partial.

As for accessing the IRibbonUI instance itself, I'm not 100% certain as to why you would need it... Beyond maybe activating a specific tab? I'm fairly new to VSTO; could you please elaborate more on how you would use the instance?

Maybe. It's been a while since I last looked at the internals of ExcelDna, so don't remember much, but based on your comments I think it is def. worth a shot, so at least the Ribbon side could have a better approach to DI.

I just tried it out and seems to work for both Excel 2002 and Excel 2007.

I currently do not have a major use for ExcelFunction or IRibbonUI so I will continue with the above approach for the time being.

@augustoproiete
Copy link
Member Author

augustoproiete commented Nov 1, 2016

@robodude666:

  1. It may be possible to use Registration as an example on how to create a custom IoC-wrapped ExcelFunction?

Last time I tried, I remember I was able to do constructor injection if creating a new instance every time (not pretty, lots more work compared to # 2) - could not find a way to do parameter injection, which is what I really wanted.

  1. Store the IoC container in a static class and access it from the ExcelFunction to get the instance for dependencies. To make it less dirty

Yep, this seems to be the least painful... Classic service locator (anti-) pattern.

As for accessing the IRibbonUI instance itself, I'm not 100% certain as to why you would need it...

If you're using MVVM, you'll want to implement Ribbon buttons as Commands in your view model, and bind the command's CanExecute to the Ribbon, so as to enable/disable or show/hide the buttons in the Ribbon. You need to manually call Invalidate in the IRibbon in order to refresh the buttons (and have them enabled/disabled or shown/hidden based on the command state).

Example scenario: A button on the Ribbon can only be enabled if there is at least one Sheet in the workbook (i.e. disable the button after the user closes all Sheets and have an empty workbook).

@robodude666
Copy link

robodude666 commented Nov 1, 2016

@augustoproiete I see from the MVVM point of view you'd want access to the IRibbonUI.

While doing some further research I ran across VSTOContrib which has a very interesting take on VSTO + IoC.

I suspect it would be possible to create all of your ExcelFunctions (and other Ribbon elements) as ViewModel classes. Then have AutoOpen programmatically create ExcelFunctions for each of your ViewModels that need 'em.

I'll dink around for a bit and see what I come up with 😄.

-robodude666

@govert
Copy link
Member

govert commented Nov 1, 2016

It should be very easy to make your own version of ExcelRibbon which is not automatically instantiated.

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

4 participants