aspnet/Mvc

Consider replacing ActivatorUtilities for TagHelper activation

Closed this issue · 6 comments

jbagga commented

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

NTaylorMullen commented

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?

jbagga commented

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

rynowak commented

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 aspnet/DependencyInjection#627