JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
566 stars 117 forks source link

Cannot register policies after container construction #246

Closed AaronAllBright closed 4 years ago

AaronAllBright commented 4 years ago

When adding configuration to a container, you cannot set policies (even though they're supposed to be additive).

container = new Container(c=>{})
container.Configure(serviceCollection => {
    var c = (ServiceRegistry)serviceCollection;
    c.Policies.SetAllProperties(a => a.OfType<IExampleService>());
});

Looking at the implementation of container.Configure, it looks like only new service registrations are added to the container configuration, but nothing else that you could normally do from the ServiceRegistry created for the constructor configuration action.

Also, the example given in the documentation: https://jasperfx.github.io/lamar/documentation/ioc/registration/

Shows

var container2 = new Container();
container2.Configure(c =>
{
    c.For<IFoo>().Use<Foo>();
    c.For<IBar>().Use<Bar>();
});

which won't compile. Hence the silly c => {} passed to the constructor and var c = (ServiceRegistry)serviceCollection; in my second configuration block above.

jeremydmiller commented 4 years ago

@AaronAllBright I really didn't want to support this kind of usage in Lamar. You need to effectively reset all of the container to make this work. I'm trying really hard to get people to stop using Configure() altogether if at all possible.

Did your PR cover this then?

AaronAllBright commented 4 years ago

Ya I can understand that. I guess the pattern that leads to this is some kind of base or common config that is added to by further derived code, or in different environments. I suppose this could be solved by better use of registries (i.e. passing environment specific registries into whatever code constructs the container, but this requires that the author of whatever component creates the initial container be very aware of how it might be extended in the future.

If you're going to expose the ability to further configure the container, then it needs to work 100% not 50%. The user will have to understand that there's a cost to the second configure.

I can think of a few ways to mitigate this cost: 1) Don't plan until first GetInstance call. 2) Come up with a way to mark dirty any instances who's planned bindings might be affected by a new addition to the container, and only re-planning those. 3) Documentation says planning is incremental, but it's definitely not. If it were, this would be a non-issue.

Anyway, yes, my PR covers this issue. Thanks for all your work on this and letting me contribute this (admittedly ham-handed) fix.

jeremydmiller commented 4 years ago

You can't just "re-plan" anytime though because singletons may already be built. My recollection is that Configure() is only there because of something wonky in early ASP.net Core v1. It's so easy just to pass around a ServiceRegistry / ServiceCollection that I'm questioning why Configure() is even necessary?

jeremydmiller commented 4 years ago

I'm closing this. I don't want to have to support this usage, and it always caused bugs in StructureMap.