Mvc: [Discussion]: ViewComponent invocation changes

[pranavkm]

As part of https://github.com/aspnet/Mvc/issues/612, we simplified the behavior around selecting, and invoking ViewComponents.

• To reduce ambiguity in ViewComponent method selection, we've modified the selection to only allow exactly one Invoke or InvokeAsync per ViewComponent.
• Component.Render, Component.RenderAsync and Component.Invoke have been removed.
• The signature of Component.InvokeAsync has been modified to accept a single argument that specifies the argument names and values used while calling ViewComponent.Invoke \ ViewComponent.InvokeAsync.
  e.g.

C# public class TestViewComponent : ViewComponent { public IViewComponentResult Invoke(string name, int age) { .... } }

Before

@Component.InvokeAsync("Test", "MyName", 15)

After

@Component.InvokeAsync("Test", new { name = "MyName", age = 15 })
or
@Component.InvokeAsync("Test", new Dictionary<string, object> { ["name"] = "MyName", ["age"] = 15 })
or
@Component.InvokeAsync<TestViewComponent>(new { name = "MyName", age = 15})

[JaymzZh]

@pranavkm @Component.InvokeAsync("Test", new Dictionary<string, object { ["name"] = "MyName", ["age"] = 15 }) should be @Component.InvokeAsync("Test", new Dictionary<string, object> { ["name"] = "MyName", ["age"] = 15 }), missing '>' ?

[pranavkm]

Yup, thanks for catching that.

[Daniel15]

Why do we need this syntax:

@Component.InvokeAsync("Test", new { name = "MyName", age = 15 })

If we have this strongly-typed one:

@Component.InvokeAsync<TestViewComponent>(new { name = "MyName", age = 15})

The "stringly-typed" version seems worse in that typos aren't caught until runtime. Why not just always use the strongly-typed one?

[davidfowl]

Because view component names could be coming from anywhere (a database, url scheme etc) and you always need a loosely typed and strongly typed version. Think of dynamic systems like cms using something like this.

[rynowak]

Why not just always use the strongly-typed one?

Generics and Razor hate each other.

[Daniel15]

Think of dynamic systems like cms using something like this.

Ah, good point. Having said that, it seems like the strongly-typed version should be the preferred syntax, right?

Generics and Razor hate each other.

How so?

[njy]

@davidfowl : "and you always need a loosely typed and strongly typed version. Think of dynamic systems like cms using something like this."

Yes, yes, YES!
A huge thanks to the entire team for thinking like this, this is fundamental.
:+1:

[rynowak]

How so?

@{ await ViewComponent.InvokeAsync<T>(...); }

Works pretty well, but

@await ViewComponent.InvokeAsync<T>(...)

will interpret <T> as an HTML tag

[dazinator]

@davidfowl :

Because view component names could be coming from anywhere (a database, url scheme etc) and you always need a loosely typed and strongly typed version.

Thanks for thinking of us CMS / framework devs! :)

I have tried to do this though, and I have noticed a few dilemmas so far.

Let's say you have a VC that's installed at runtime, by say, dropping the assembly containing the VC into some folder location. When your site starts up it scans that location and loads those assemblies dynamically and registers them with MVC appropriately.

Now let's say in layout.cshtml, you want to render some pluggable VC, but only if it exists..

• In the razor view, at design time, you get some indication of a problem because it says it can't find the VC:

That's expected I guess, because the VC will only exist at runtime when it has been installed.

• When the application starts, before you have installed the the VC, your application will error because layout is trying to render a VC that cannot be found.

In order to solve this, it seems to me like we need some way of determining if a VC exists or not, prior to attempting to Invoke() it. Or, we need some sort of InvokeIfExists() API.

Basically, we want to treat the presence of a VC as optional and for things not to break.

Do you have a recommended path in MVC 6 to achieving that currently?

Lastly, I did find a problem with loading up VC's at runtime. It seems that a project / assembly reference to the VC assembly is currently mandatory. Here is my repro / demonstration of that

[Antaris]

@dazinator It is possible to develop your own API to do that InvokeIfExists check:

public interface IExtendedViewComponentHelper
{
  Task<HtmlString> InvokeIfExistsAsync(string name, object arguments = null);

  Task<HtmlString> InvokeIfExistsAsync(Type componentType, object arguments = null);
}

public class ExtendedViewComponentHelper : IExtendedComponentHelper
{
  private readonly IViewComponentDescriptorCollectionProvider _descriptorProvider;
  private readonly IViewComponentSelector _selector;
  private readonly IViewComponentHelper _helper;

  public ExtendedViewComponentHelper(
    IViewComponentDescriptorCollectionProvider descriptorProvider,
    IViewComponentSelector _selector,
    IViewComponentHelper _helper)
  {
    _descriptorProvider = descriptorProvider;
    _selector = selector;
    _helper = helper;
  }

  public Task<HtmlString> InvokeIfExistsAsync(string name, object arguments = null)
  { 
    return Exists(name)
      ? _helper.InvokeAsync(name, arguments)
      : Task.FromResult(new HtmlString(string.Empty));
  }

  public Task<HtmlString> InvokeIfExistsAsync(Type componentType, object arguments = null)
  {
    return Exists(componentType)
      ? _helper.InvokeAsync(componentType, arguments)
      : Task.FromResult(new HtmlString(string.Empty));
  }

  private bool Exists(string name)
  {
    return _selector.SelectComponent(name) != null;
  }

  private bool Exists(Type componentType)
  {
    var componentTypeInfo = componentType.GetTypeInfo();
    var descriptors = _descriptorProvider.ViewComponents;
    for (int i = 0; i < descriptors.Items.Count; i++)
    {
      var descriptor = descriptors.Items[i];
      if (descriptor.TypeInfo = componentTypeInfo)
      {
        return true;
      }
    }

    return false;
  }
}

@inject IExtendedViewComponentHelper ComponentEx

@await ComponentEx.InvokeIfExistsAsync("<name>", { key = "value" })

[dazinator]

@Antaris - ahh that's brilliant! Thank you.

[dazinator]

@Antaris - I am seeing this exception attempting to get this working:

@await Component.InvokeAsync("NavMenu") works fine.
@await ViewComponentHelper.InvokeIfExistsAsync("NavMenu") hits this error.

The Exists() check is finding the component fine, and its calling through to _helper.InvokeAsync(name, arguments) which is the DefaultViewComponentHelper - and then somewhere in that method it looks like it cannot construct a ViewComponentContext - but I have no idea why.

Could this be to with the fact that @inject is forcing the creation of an DefaultViewComponentHelper too early, before some context is available? I'm going to try and delve into the code to figure it out but I could use some ideas

[dazinator]

@Antaris - Turns out the Injected instance of DefaultViewComponentHelper does not have it's Contextualize method called, so it never recieves a ViewContext and therefore as soon as you invoke it, it hits the null ref exception I have mentioned above.

To solve this, I removed the constructor injection for that instance, and pass in the IViewComponentHelper instance from the razor view, as that instance does get ViewContext appropriately. Here is a working sample for anyone that interested:

public class ViewComponentHelper : IExtendedViewComponentHelper
    {
        private readonly IViewComponentDescriptorCollectionProvider _descriptorProvider;
        private readonly IViewComponentSelector _selector;

        public ViewComponentHelper(
          IViewComponentDescriptorCollectionProvider descriptorProvider,
          IViewComponentSelector selector)
        {
            _descriptorProvider = descriptorProvider;
            _selector = selector;
        }

        public Task<HtmlString> InvokeIfExistsAsync(IViewComponentHelper componentHelper, string name, object arguments = null)
        {
            if (!Exists(name))
            {
                return Task.FromResult(new HtmlString(string.Empty));
            }

            if (arguments == null)
            {
                return componentHelper.InvokeAsync(name);
            }

            return componentHelper.InvokeAsync(name, arguments);

        }

        public Task<HtmlString> InvokeIfExistsAsync(IViewComponentHelper componentHelper, Type componentType, object arguments = null)
        {
             if (!Exists(componentType))
            {
                return Task.FromResult(new HtmlString(string.Empty));
            }

            if (arguments == null)
            {
                return componentHelper.InvokeAsync(componentType);
            }

            return componentHelper.InvokeAsync(componentType, arguments);
        }

        private bool Exists(string name)
        {
            var comp = _selector.SelectComponent(name);
            bool exists = comp != null;
            return exists;
        }

        private bool Exists(Type componentType)
        {
            //  var componentTypeInfo = componentType.GetTypeInfo();
            var descriptors = _descriptorProvider.ViewComponents;
            for (int i = 0; i < descriptors.Items.Count; i++)
            {
                var descriptor = descriptors.Items[i];
                if (descriptor.Type == componentType)
                {
                    return true;
                }
            }

            return false;
        }
    }

Then you call it from the view as before, but now you pass the additional arg for the IViewComponentHelper:

@await ViewComponentHelper.InvokeIfExistsAsync(Component, "NavMenu")

[Antaris]

@dazinator What about this:

public class ExtendedViewComponentHelper : IExtendedViewComponentHelper, ICanHasViewContext
{
  // Other code removed for brevity...

  public void Contextualize(ViewContext viewContext)
  {
    (_viewComponentHelper as ICanHasViewContext)?.Contextualize(viewContext);
  }
}

I haven't quite figured out the magic of ICanHasViewContext in respect to the page activator, but it just _works_ for the other types.

[rynowak]

The above code with ICanHasViewContext will do what you want inside a Razor page.

ICanHasViewContext is magic that the Razor @inject activator knows about. We're going to try and replace this with something better before we ship RC2.

[dazinator]

@Antaris - thanks, i thought about attempting that but decided not to bother as I would prefer to reuse the views instance and not create a duplicate for now. Also if stuff around ICanHasViewContextis potentially changing in RC2 probably best to avoid interacting with it for now.
Thanks for your help with this, i'm loving view components so far.

[Antaris]

@rynowak What would the alternative be, I'm guessing something like a type where you lease ViewContext from an object pool perhaps, and stack them up for each View -> PartialView -> PartialView etc?

[rynowak]

I'm guessing something like a type where you lease ViewContext from an object pool perhaps

We've played with designs where ViewContext is part of the DI system.
1). It's not something we're doing in other parts of the framework (why can't I DI the HttpContext?)
2). It can imply another layer of magic that other DI systems don't have
3). We don't like magic

The proposal floating around internally was to do something like this (using IViewComponentHelper as the example).

Framework Code:

public interface IViewContextAwareFactory<out T>
{
    T Create(ViewContext viewContext);
}

public interface IViewComponentHelperFactory : IViewContextAwareFactory<T>
{
}

public class DefaultViewComponentHelperFactory : IViewComponentHelperFactory
{
    public IViewComponentHelper Create(ViewContext viewContext)
    {
        // Code in contextualize moved to the constructor YAY
        return new DefaultViewComponentHelper(...); 
    }
}

Getting a VC helper in arbitrary code

var factory = ...; // factory comes from DI
var helper = factory.Create(ViewContext); // Need to have or make a ViewContext

This is just plain old CSharp code. The only thing special about it is that you have to know that you want the factory instead of the instance directly.

Getting a VC helper in Razor

@inject IViewComponentHelper ViewComponent

OK - this does have a layer of _magic_ - we're generating code here that looks something like:

private IViewComponentHelper _viewComponent;

public IViewComponentHelper ViewComponent
{
    get
    {
        if (_viewComponent == null)
        {
            _viewComponent = _services.GetService<IViewComponentHelper>();
        }
    }
}

We'll likely also allow this to generate code like:

private IViewComponentHelper _viewComponent;

public IViewComponentHelper ViewComponent
{
    get
    {
        if (_viewComponent == null)
        {
            var factory = _services.GetService<IViewComponentHelperFactory>();
            _viewComponent = factory.Create(ViewContext);
        }
    }
}

I think _we_ are a little more ok with the magic in this case because it's not code you get to write in Razor.

We have a few APIs like this in MVC - where a replaceable service has closed over the current context (IUrlHelper, IHtmlHelper). Right now the factory pattern for IUrlHelper is hardcoded in Razor. We want to generalize this and then use it for others components as well.

[Antaris]

Hmm, would you need to register the DefaultViewComponentHelperFactory as both an IViewComponentHelperFactory and an IViewContextAwareFactory<IViewComponentHelper> in your DI registrations?

I guess the other bit of magic is getting from an IViewComponentHelper to an IViewComponentHelperFactory when generating with @inject too

[rynowak]

Yeah that's likely the case, which I'll admit isn't optimal. we're working out the details

[mpawelski]

@rynowak What about the idea you wrote on #612:

Also interesting to bring back the idea of an expression-based syntax. We discussed this but stopped short of committing to build it.

@Component.Invoke(cloud => cloud.ShowTags(count: 10))

I also had similar idea and wrote about it on #3900.
I think this syntax is great even without any special Razor support (just wrap this call with @() to avoid confusion about < character). What is team opinion about it? Is is consider to ship before RTM?

[rynowak]

What is team opinion about it?

We still think it's a credible idea, but it's a new feature and something we're not likely to revisit before RTM (hence backlog).

[pranavkm]

We attempted @rynowak's design as part of https://github.com/aspnet/Mvc/issues/4073. However there are limitations in the DI system that disallowed defining \ consuming some variants of IViewContextAwareFactory in our code. We couldn't come up with anything better at this stage so we stuck with the pattern we have. The type Microsoft.AspNetCore.Mvc.Rendering.Internal.ICanHasViewContext was renamed to Microsoft.AspNetCore.Mvc.Rendering.IViewContextAware.

[andrew-vdb]

I'm bit late to the party
But this decision impacting my code :(

It is silly to only allow one method in a class anyway

public class TextViewComponent : ViewComponent
{
        public IViewComponentResult Invoke(TextBoxModel model)
        {
            return View("~/textbox.cshtml", model);
        }

        public IViewComponentResult Invoke(TextAreaModel model)
        {
            return View("~/textarea.cshtml", model);
        }
}

I believe it's logical and nice OOP

Now I have to change it into

public class TextViewComponent : ViewComponent
{
        public IViewComponentResult Invoke(TextViewModel model)
        {
           switch(model.TypeName){
           case "TextBox":
           return View("~/textbox.cshtml", model);
           case "TextArea":
            return View("~/textarea.cshtml", model);
        }        
}

Or of course to create class per ViewComponent

I know we just on RTM, allowing more than one Invoke won't give breaking change, is it possible to go back when we can have more than one Invoke in ViewComponent?

[njy]

@andrew-vandenbrink

I know we just on RTM

I think there you have your answer :)

[andrew-vdb]

@njy well there will be 1.1 right :)

[andrew-vdb]

To my surprise invoking ViewComponent like this not working

@Component.InvokeAsync("TextView", new TextViewModel() {  }) // NOT working anymore, no compiler check, it just suddenly null when you run it

This way is working but very not intuitive and awkward

@Component.InvokeAsync("TextView", new{model=new TextViewModel() {  }}) 

We have milliseconds performance gain but more eye shore

I'm also not sure the idea behind removing Invoke
Not all ViewComponent designed to be async

ViewComponent in RC1 is much better than what we have in RTM

[psulek]

@andrew-vandenbrink Just hit problem you mentioned that i have passed my ViewModel directly into InvokeAsync and then in my view component (in Net Core 1.0 RTM) parameter is null !!

[andrew-vdb]

@psulek I decide not to use ViewComponent

[psulek]

I dont want to refuse ViewComponent. It is good feature especially from Net Core RC2 and latter (RTM) you can have view components embed in separate assembly and than load in other web app (see post about it here - Re-using external view components in ASP.NET 5 & ASP.NET MVC 6

[rynowak]

@andrew-vandenbrink

The reason we removed support for multiple methods (overloading) is that we can't implement what the C# compiler does for overload selection and conversions of arguments. What we shipped in RC1 was a bad approximation and lots of people found holes in it.

If your code is really as simple as what you provided in the example you might want to look into display/editor templates http://www.growingwiththeweb.com/2012/12/aspnet-mvc-display-and-editor-templates.html

This does support "fanning out" based on model type.

I'm also not sure the idea behind removing Invoke. Not all ViewComponent designed to be async

The removal of Invoke was to simplify the API for calling a VC from a view. You used to have to remember which ones were safe to call synchronously, and even then IViewComponentResult can do async work. There wasn't really any reason for it to exist and the behavior of overloading based on sync/async was complicated to explain. If we had never provided Invoke, would you be asking for us to add it?

Regarding some of the feedback about invoking VCs, we're looking into two changes here.

We still want to find a way to codegen view components as TagHelpers. That will provide the most concise and intellisense-friendly way of calling them. Generics and Razor fight, so anything using expressions can be equally cumbersome. This is a long distance goal, and we don't have a timeline to share.

In the near future, we're also considering supporting the following. Both:

@await Component.InvokeAsync("TextView", new TextViewModel() {  }) 

and

@await Component.InvokeAsync("TextView", new { model = new TextViewModel() {  } }) 

The restriction is that the former can be used when your VC takes a single parameter, and the provided model type can be assigned to the parameter type.

The latter is the currently supported syntax and would still be required when you have multiple parameters.

[dazinator]

At the moment, if you call a VC like this:

@await Component.InvokeAsync("TextView", new TextViewModel() {  }) 

And you have an InvokeAsync method on your VC that takes an argument:

        public IViewComponentResult Invoke(TextViewModel model)
        {
            return View("~/textview.cshtml", model);
        }

We now get null passed in.

I think it would be much better to throw an appropriate exception, like an Argument Mismatch exception.

The invoker is sending an argument so there is an obvious expectation for that argument to make it to the view component. The view component is accepting an argument - so again there is an obvious assumption that it will recieve something. Currently MVC says - "meh you didn't use new { }" so I'll just pass in null. This is pain point for those of us not defensively coding with null checks in our VC's. In this scenario, why doesn't MVC say - "I can't actually supply the argument to this View Component, so throw an exception indicating the problem.". To assume that we want null passed in is not a safe assumption to make when a value was actually supplied with the original invoke - all be it with the wrong syntax.

[rynowak]

@dazinator - that's something we could do as well. We'd really like to make sure we're doing the best we can for usability as part of the same work.

[andrew-vdb]

@rynowak

I port it into Partial View

@Component.Invoke("TextView", new TextViewModel() {  }) //In future it will works,maybe

@Html.Partial(TextViewModel.ViewName, new TextViewModel(){}) //Works fine today

Display and Editor template is poor choice in my use case due the need to use of model in the view page to makes it feels right to use it. I don't want to create model for every view page I have.

If we had never provided Invoke, would you be asking for us to add it?

Well there is Html.Partial, Html.PartialAsync, so why not?
Not all my controller's code is IO bound, its normal to have CPU bound calculation too

[Eilon]

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.