aspnet/DependencyInjection

Consider replacing ActivatorUtilities for TagHelper activation

mkArtakMSFT opened this issue · 6 comments

mkArtakMSFT commented

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

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

@jbagga, what are the scenarios this would enable?

mkArtakMSFT commented

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

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

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

This issue was moved to dotnet/aspnetcore#2871