aspnet / DependencyInjection

[Archived] Contains common DI abstractions that ASP.NET Core and Entity Framework Core use. Project moved to https://github.com/aspnet/Extensions
Apache License 2.0
873 stars 318 forks source link

Consider replacing ActivatorUtilities for TagHelper activation #627

Closed mkArtakMSFT closed 6 years ago

mkArtakMSFT commented 6 years ago

From @jbagga on February 5, 2018 21:54

For cases that need a new constructor for a class (additional param), in order to avoid a breaking change we add an overload. However, ActivatorUtilities does not allow multiple applicable constructors.

Message: System.InvalidOperationException : Multiple constructors accepting all given argument types have been found in type 
'Microsoft.AspNetCore.Mvc.Razor.TagHelpers.HeadTagHelper'. There should only be one applicable constructor.

This restricts TagHelpers from changing the constructors.

cc @rynowak

Copied from original issue: aspnet/Mvc#7330

mkArtakMSFT commented 6 years ago

From @NTaylorMullen on February 5, 2018 22:22

I'd think if we wanted to support multiple ctors we'd take an approach of decorating the constructor we'd want to use with some sort of attribute to say "Use this constructor when activating". Other DI systems do this already.

Granted I think it'd be a bigger change outside of MVC if we were going to do this (don't think it's worth doing for just TagHelpers).

mkArtakMSFT commented 6 years ago

@jbagga, what are the scenarios this would enable?

mkArtakMSFT commented 6 years ago

From @jbagga on February 6, 2018 17:59

Multiple constructors for TagHelpers (and also other types that use ActivitorUtilities in MVC to create instances). But @rynowak said it was particularly problematic for TagHelpers

mkArtakMSFT commented 6 years ago

From @rynowak on February 7, 2018 3:17

We have created a potential problem for our users everywhere that we use ActivatorUtilities. This includes things like tag helpers, controllers, view components, page models, filters, etc.

ActivatorUtilities only supports a single constructor (in our use cases).

This means that if you both:

  1. Need to add constructor parameters
  2. Care about making breaking changes to your API

Then you have a problem.

This is particularly bad for tag helpers since we intend for it to be possible for someone to ship them as a library. This is also bad for everything else, since shipping those in libraries is a thing as well.


property injection is the best injection IMO

mkArtakMSFT commented 6 years ago

We just had a meeting about this and the solution we will go forward for ActivatorUtilities will be to take the only longest available public constructor for instantiation.

mkArtakMSFT commented 6 years ago

This issue was moved to aspnet/Home#2871