Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ViewComponentTagHelper: Allow optional parameters #5011

Closed
pawchen opened this issue Apr 28, 2017 · 45 comments
Closed

ViewComponentTagHelper: Allow optional parameters #5011

pawchen opened this issue Apr 28, 2017 · 45 comments
Assignees
Labels
affected-medium This issue impacts approximately half of our customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views feature-razor-pages severity-major This label is used by an internal tool
Milestone

Comments

@pawchen
Copy link

pawchen commented Apr 28, 2017

Say we have a ViewComponent class

class MyViewComponent
{
    IViewComponentResult Invoke( bool showSomething = false ) { ... }
}

It would be great to just only write <vc:my /> in Razor if one don't want to show that something. Currently you have to write <vc:my show-something="false" />.

@DamianEdwards
Copy link
Member

Agreed. No idea how feasible it is, but I agree with the sentiment 😄

@rynowak
Copy link
Member

rynowak commented Apr 28, 2017

This seems pretty feasible. We need to teach this code about default values. Currently it says that each parameter is a required attribute

@DamianEdwards
Copy link
Member

@rynowak candidate for preview2?

@rynowak
Copy link
Member

rynowak commented Apr 29, 2017

I'm going to move it in and mark it up for grabs. At this point locking down our public APIs and completing the tooling story needs to take priority.

@NTaylorMullen NTaylorMullen changed the title ViewComponentTagHelper: Allow omitting attribute for parameters with default value ViewComponentTagHelper: Allow optional parameters Jun 29, 2017
@svallis
Copy link

svallis commented Jun 29, 2017

Really looking forward to this fix. As things stand, using the VC tag helper is very dangerous. Add a new parameter to an existing and widely used tag helper and miss updating one of the places it is called and it silently fails while emitting server side markup to the browser. I guess we're too far along now for this to make it in to 2.0.0?

@rynowak
Copy link
Member

rynowak commented Jun 29, 2017

This is not going to make it for 2.0.0. We don't have time to update tooling at this point.

@Liero
Copy link

Liero commented Oct 21, 2017

I believe it should be marked as a bug.

This is working:

@await Component.Invoke(typeof(MyViewComponent));

This is not working

<vc:My />

It is expected behavior and since you do not show any warning, error, debug message, nothing, it upsets a lot of people wondering why the hell the view component is not rendered

@clockwiseq
Copy link

So will this make it into the next release?

@dseipp
Copy link

dseipp commented Nov 2, 2018

Looking for an update on this issue. Will this be added any time soon? Rather frustrating one to deal with.

@aspnet-hello aspnet-hello transferred this issue from aspnet/Razor Dec 14, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 14, 2018
@aspnet-hello aspnet-hello added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature feature-razor-pages help wanted Up for grabs. We would accept a PR to help resolve this issue labels Dec 14, 2018
@ASKemp
Copy link

ASKemp commented May 16, 2019

Came here to say same thing after finding this issue and spending some time tracking down why on of my tag helper called VC's wasn't presenting any output.

@CamiloTerevinto
Copy link

Hey @rynowak:

I thought I would give this a shot and after reading the source code here (your link is dead), it seems pretty easy to fix. However, there are at least two ways ( either create a replica of RequiredAttributeDescriptor and friends, or move to a base clas/interface) to do this and it's a substantial amount of code changes.

@emumanu
Copy link

emumanu commented Jul 23, 2019

@rynowak and/or @DamianEdwards, please tell something to @CamiloTerevinto as we want to see this improved.

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Jul 29, 2019

Updated View components in ASP.NET Core with

All view component parameters are required

Each parameter in a view component is a required attribute. See this GitHub issue. If any parameter is omitted:

  • The InvokeAsync method signature won't match, therefore the method won't execute.
  • The ViewComponent won't render any markup.
  • No errors will be thrown.

@lvmajor
Copy link

lvmajor commented Oct 23, 2019

Just got bitten hard by it (the notice was not there at the time I implemented it in my app)

Is there a planned solution or will it stay like this for the foreseeable future? As this can be really dangerous, especially that it fails silently... Could there at least be a warning or something like that at compile time?

@jerriep
Copy link

jerriep commented Oct 30, 2019

Yeah, I agree with the last comment. I also got bitten hard by this recently and the fact that it simply fails silently is not good.

@Rick-Anderson
Copy link
Contributor

@rynowak is it possible to generate an error?

@staa99
Copy link

staa99 commented Nov 12, 2019

@rynowak Will there be any support for this in the future?

It's been over two years, and honestly, it is a pain to have to duplicate default parameters everywhere a view component is used.

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Nov 12, 2019

@rynowak what's the correct line number in

This seems pretty feasible. We need to teach this code about default values. Currently it says that each parameter is a required attribute

this code : updated link but maybe not correct line

@josephr5000
Copy link

Thanks @Rick-Anderson for updating the docs, but it's ridiculous that you should even have to. This makes the tag helper UNUSABLE for serious projects. To have code fail silently when you refactor or add features should be completely unacceptable. I'm stunned that this is still open almost three years after first being reported. Is any fix planned?!

@clockwiseq
Copy link

Has anyone from the aspnetcore team looked at this and given an ultimate answer as to whether or not this will be implemented? I personally think it's something that makes view components actually useful and would reduce redundant code. At bare minimum, I would like to see someone from the team take ownership and make a decision on the fate of this feature.

@Rick-Anderson
Copy link
Contributor

@mkArtakMSFT @rynowak please review and @rynowak can you check

@rynowak what's the correct line number in

This seems pretty feasible. We need to teach this code about default values. Currently it says that each parameter is a required attribute

this code : updated link but maybe not correct line

@ghost
Copy link

ghost commented Nov 16, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@jishansiddique
Copy link

This issue exists in .NET 5 as well as.
Any update?

@mc0re
Copy link

mc0re commented Feb 12, 2021

Also note, that this syntax works fine for tag helpers (as it does in ordinary HTML and in XAML -and I'm probably missing a dozen of other examples). I wanted to switch from a tag helper to a view component, as my HTML grew, and I didn't want to generate it in code. Now I have to add a lot of unnecessary HTML in all the other places :-(

@Santas
Copy link

Santas commented Feb 24, 2021

Is there a plan to land this in .NET 6? Thanks!

@jishansiddique
Copy link

Hi @Santas,
As per the blog Click me,
I've checked but not able to find any optional parameter in the view component.

@OlegWald
Copy link

we really need optional parameters support in ViewComonents. please give us this

@javiercn javiercn added the feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views label Apr 19, 2021
@mkArtakMSFT
Copy link
Member

Let's try to time-box this effort. If we can do this in 2 days, we'll do this in current milestone (6.0-preview6)

@mkArtakMSFT mkArtakMSFT added this to 6.0-preview6 (CC: 15th June) in ASP.NET Core Blazor & MVC 6.0 May 17, 2021
@lvmajor
Copy link

lvmajor commented May 17, 2021

Let's try to time-box this effort. If we can do this in 2 days, we'll do this in current milestone (6.0-preview6)

This would be really awesome!

@captainsafia captainsafia moved this from 6.0-preview6 (CC: 15th June) to In Progress in ASP.NET Core Blazor & MVC 6.0 May 19, 2021
@ghost ghost added the Working label May 19, 2021
captainsafia added a commit that referenced this issue May 29, 2021
…32979)

Addresses #5011.

To implement support for optional parameters, we largely needed to do two things:

- Update the `ViewComponentTagHelperDescriptorFactory` that is responsible for constructing `VCTHD` objects to _not_ produce the `RequiredAttributeDescriptors` for parameters that have a default value provided.
- Update the generated code for VCTHs to produce an arguments object to the `InvokeCoreAsync` method that contains only the provided parameters

**Updates to the VCTH descriptor factory**

The VCTH descriptor contains a `RequiredAttributeDescriptor` for every parameter to the `InvokeAsync` method on the VC. When a parameter has a default value provided, we don't need to configure the parameter as a `RequiredAttribute` on the VCTH descriptor.

With this change in place, instances of the VCTH that don't contain the required attribute will still match with the VCTH. For example, the following VC:


```csharp
public class FooViewComponent : ViewComponent
{
    public IViewResult InvokeAsync(string foo, string bar = "default value")
    {
        return $"{foo} : {bar}";
    }
}
```

Would produce a VCTH descriptor that looks like this:

```
- ViewComponentTagHelper
  - RequiredAttributeDescriptor(propertyName = "foo")
  - BoundAttributeDescriptor(propertyName = "foo")
  - BoundAttributeDescriptor(propertyName = "bar")
```

And will match VCTH instances like:

```
<vc:foo foo="a-foo-value"></vc:foo>
<vc:foo foo="a-foo-value" bar="a-bar-value"></vc:foo>
```

But not VCTH instances that looks like:

```
<vc:foo bar="a-bar-value"></vc:foo>
<vc:foo></vc:foo>
```

All this can happen with very few changes to the tag helper matching logic that already exists.

**Update the generated code for VCTHs**

Presently, whenever a VCTH is encountered, generated code is produced that:

- Writes a class representing the view component which contains properties for each parameter provided to the `InvokeAsync` method and a `ProcessAsync` method that passes the class properties as arguments to `InvokeAsync`
- Instantiates an instance of the class above and sets class properties with their associated attributes

Even if the VCTH is matched to the VC successfully as a result of the changes above, the generated code will be inaccurate because null values will be passed for parameters that have default values. Instead, we want to construct an arguments object that only contains the parameters that were provided as attributes to the VCTH instance.

We do this by adding a new `ProcessInvokeAsyncArgs` that evaluates if each of the parameters was provided as an attribute to the VCTH instance and if so, sets its value in the arguments object.

```csharp
private Dictionary<string, object> ProcessInvokeAsyncArgs(Microsoft.AspNetCore.Razor.TagHelpers.TagHelperContext __context)
{
    Dictionary<string, object> args = new();
    if (__context.AllAttributes.ContainsName("first-name"))
    {
        args[nameof(firstName)] = firstName;
    }
    return args;
}
```

The net effect is that we only pass set parameters as arguments to the method. For parameters that aren't passed as arguments, the default value will be used.
@captainsafia
Copy link
Member

Closing as resolved. This will ship in .NET 6 Preview 6. Thanks for the patience folks!

@JeremyCaney
Copy link

Oh, I’m so happy to hear that, @captainsafia! This will make view components a lot more flexible—and, especially, when distributed as part of e.g. Razor Class Libraries, where the lack of either intuitive defaults or obvious error messages has made them cumbersome for implementors and a headache from a support perspective. Thank you for taking the time to complete this!

ASP.NET Core Blazor & MVC 6.0 automation moved this from In Progress to Done Jun 2, 2021
@ghost ghost added Done This issue has been fixed and removed Working labels Jun 2, 2021
@tompazourek
Copy link

This is great. Thank you all very much.

I have one a bit related/unrelated question: How do you update the documentation pages so that they'll reflect the new features and changes? Should there be a PR in https://github.com/dotnet/AspNetCore.Docs to update the information there?

This part is now no longer true, right?

All view component parameters are required
Each parameter in a view component is a required attribute.
(...)

https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/mvc/views/view-components.md#all-view-component-parameters-are-required

@jishansiddique
Copy link

This is really great. Thank you all very much.

@captainsafia
Copy link
Member

@tompazourek Good call.

@Rick-Anderson We can update the docs here for .NET 6 on as this feature is now supported. If a default value is passed to a parameter, then that parameter is option.

@Rick-Anderson
Copy link
Contributor

If a default value is passed to a parameter, then that parameter is option.

then that parameter is option optional.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-mvc-razor-views Features related to the Razor view engine for Razor pages and MVC views feature-razor-pages severity-major This label is used by an internal tool
Projects
No open projects
Development

No branches or pull requests