JasperFx / lamar

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

Fix support for named parameters with constructor injection #327

Closed dougbenham closed 2 years ago

dougbenham commented 2 years ago

Currently named parameters only affect QuickBuild, but this PR should fix it when using GetInstance or auto-injection into constructors.

rizi commented 2 years ago

@dougbenham could you please provide a sample what's now working that wasn't working before.

Br

dougbenham commented 2 years ago

In the quick build tests you will find this code:

    public class SelectiveWidgetUser
    {
        public IWidget Widget { get; }

        public SelectiveWidgetUser([Named("green")]IWidget widget)
        {
            Widget = widget;
        }
    }
        [Fact]
        public void honor_named_attribute_on_parameter()
        {
            var container = new Container(_ =>
            {
                _.For<IWidget>().Use<GreenWidget>().Named("green");
                _.For<IWidget>().Use<BlueWidget>().Named("blue");
            });

            container.GetInstance<IWidget>().ShouldBeOfType<BlueWidget>();

            container.QuickBuild<SelectiveWidgetUser>()
                .Widget.ShouldBeOfType<GreenWidget>();
        }

It doesn't work if you try using:

            container.GetInstance<SelectiveWidgetUser>()
                .Widget.ShouldBeOfType<GreenWidget>();

and I think we can agree that QuickBuild is supposed to be interchangeable with GetInstance, at least in this scenario. So basically what I'm saying is standard GetInstance calls (and constructor injection) do not care about Named parameters currently.

rizi commented 2 years ago

In the quick build tests you will find this code:

    public class SelectiveWidgetUser
    {
        public IWidget Widget { get; }

        public SelectiveWidgetUser([Named("green")]IWidget widget)
        {
            Widget = widget;
        }
    }
        [Fact]
        public void honor_named_attribute_on_parameter()
        {
            var container = new Container(_ =>
            {
                _.For<IWidget>().Use<GreenWidget>().Named("green");
                _.For<IWidget>().Use<BlueWidget>().Named("blue");
            });

            container.GetInstance<IWidget>().ShouldBeOfType<BlueWidget>();

            container.QuickBuild<SelectiveWidgetUser>()
                .Widget.ShouldBeOfType<GreenWidget>();
        }

It doesn't work if you try using:

            container.GetInstance<SelectiveWidgetUser>()
                .Widget.ShouldBeOfType<GreenWidget>();

and I think we can agree that QuickBuild is supposed to be interchangeable with GetInstance, at least in this scenario. So basically what I'm saying is standard GetInstance calls (and constructor injection) do not care about Named parameters currently.

Can someone explain me when to use GetInstance<>() and when to use QuickBuild()?

What's the difference between this methods?

Thx in advance

dougbenham commented 2 years ago

QuickBuild is equivalent except that it is..

suitable for building concrete types that will be resolved only a few times to avoid the cost of having to register or build out a pre-compiled "build plan" internally.

jeremydmiller commented 2 years ago

They should be 100% functionally equivalent, but QuickBuild() works with Reflection only, whereas GetInstance() is generally using a compiled lambda for construction. QuickBuild() is used behind the scenes for building any kind of singleton because the cost of building/compiling the lambda is more than the perf hit from using pure reflection. So no, you wouldn't typically ever use QuickBuild() in real usage. Maybe for doing a one time build of concrete types that aren't previously registered, but you're outside of idiomatic usage if that really comes up.

jeremydmiller commented 2 years ago

@dougbenham The code looks fine, but I'd want a previously failing test with the PR to lock this down before I pull this into master.

dougbenham commented 2 years ago

Ok I added a test for it, thank you!

dougbenham commented 2 years ago

Are there any kind of nightly builds or beta nuget packages for Lamar where I could use this?

jeremydmiller commented 2 years ago

@dougbenham It's in Lamar v8.0