dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.5k stars 10.04k forks source link

TypeConverter for parameters #8493

Closed arivoir closed 4 years ago

arivoir commented 5 years ago

I'm am trying to add a parameter of type "C1Color" I created to a razor component, and I'd like to pass it as a string.

public class C1Border : C1View
{
    [Parameter]
    private C1Color Color { get; set; }
}
<C1Border Color="#FF0000">
    <Component1 />
</C1Border>

Is there any way to use System.ComponentModel.TypeConverter for these scenarios?

[Parameter]
[TypeConverter(typeof(C1ColorTypeConverter))]
private C1Color Color { get; set; }

I know I can do something like this

<C1Border Color="@C1Color.FromHexString("#FF0000")">
    <Component1 />
</C1Border>

But I want to avoid it.

mkArtakMSFT commented 5 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to happen for the coming release. We will reassess the backlog following the current release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

rynowak commented 5 years ago

This has been added in preview 7 - https://github.com/aspnet/AspNetCore/pull/10730 Note that preview 6 is the next release so you will have to wait a few weeks.

arivoir commented 5 years ago

How will this finally work? adding the type converter attribute in each property, or can the attribute be specified in the class so it's globally taken?

rynowak commented 5 years ago

Implement ToString() on your class, and then implement a TypeConverter and register with the attribute on your class.

arivoir commented 5 years ago

I'm clear about registering the TypeConverter on the class, this way the converter can parse the string, but why is the ToString necessary?

rynowak commented 5 years ago

ToString is used to convert the value to a string. We don't use the type converter at this point because we don't easily know what cases require it.

arivoir commented 5 years ago

The idea of using the TypeConverter is to parse the string to the actual class, not the other way round. Can you clarify? I'm really confused by your statement. Will TypeConverter work or not?

rynowak commented 5 years ago

Yes, the type converter is used in parsing.

rynowak commented 5 years ago

While writing the release notes we realized this issue was closed as done by mistake. @bind-Value and other @bind related constructs support type converters.

We do not do any conversions when setting component parameters directly.

egil commented 5 years ago

An (I think) elegant solution is to use the implicit conversion/cast operator in your C1Color class. In that scenario, create an implicit cast from string to C1Color. See more here: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/user-defined-conversion-operators

With that, I am not sure what additional benefit TypeConverters have? Do they support interfaces? In that case, they can do stuff implicit operators cannot.

arivoir commented 5 years ago

@egil The implicit operator is not working as expected. Despite so, it improves a little the razor assignment as now I can omit part of the class and method

Previously GridLinesColor="@C1Color.Parse("#FFD0D7E5")"

Using implicit operator GridLinesColor="@("#FFD0D7E5")"

Desired GridLinesColor="#FFD0D7E5"

@rynowak When can we expect this to be addressed?

rynowak commented 5 years ago

When can we expect this to be addressed?

I don't know that we'd ever do something like this. One of the advantages of Blazor is that you write C# code. I don't feel like Color.Parse(...) has significant drawbacks.

Using type converters for parameters is something that's done in other languages like XAML that don't let you write code.

arivoir commented 5 years ago

Using type converters for parameters is something that's done in other languages like XAML that don't let you write code.

What do you mean? Why there are some classes like string or int that can be assigned directly without the "@" and others like enums or custom classes that need them?

rynowak commented 5 years ago

I'm not sure from the context if you're talking about Razor or XAML.

In Razor strings are special because attributes naturally use the same syntax as a .NET string. Otherwise you'd end up writing something like the following.


<div class="@("foo")" >
...
</div>
arivoir commented 5 years ago

Please compare the readability of one code and the other and tell me what's easier to read image image

arivoir commented 5 years ago

In Razor strings are special because attributes naturally use the same syntax as a .NET string. Otherwise you'd end up writing something like the following.

<div class="@("foo")" >
...
</div>

Also bool and int are special, why only those? Why not enums? It looks the problem was resolved for a small set of types, but it persist for enums and custom types

rynowak commented 5 years ago

Also bool and int are special, why only those? Why not enums?

No, I have to disagree. Those are using the normal C# syntax for bools and ints. Likewise, you use the normal C# syntax for enums and other types.

arivoir commented 5 years ago

Those are using the normal C# syntax for bools and ints.

In preview7 that's incorrect, bool's can be specified using the c# syntax by appending the "@" before "true" or "false" terms, but they are also "special" and can be set without using the "@". My point is not only those types will benefit from being "special", other types too, especially enums. In this end this will help readability of razor files.

rynowak commented 5 years ago

Whether or not an @ is required inside a component attribute is a function of the type of the declaring parameter. String-typed parameters are text by default because otherwise the syntax would be gross.

<MyComponent 
  BoolParameter="<this is C#>" 
  StringParameter="<this is text>" 
  AnotherStringParameter="@<now this is C#>"/>
arivoir commented 5 years ago

String-typed parameters are text by default because otherwise the syntax would be gross.

The same reasoning would apply for any other type that has an implicit string cast operator. The syntax end up being gross. I'm still trying to understand where the reluctancy to improve this scenario comes from. I understand there could be reasons, but I haven't heard the arguments yet.

rynowak commented 5 years ago

I'm still trying to understand where the reluctancy to improve this scenario comes from. I understand there could be reasons, but I haven't heard the arguments yet.

My reluctance is that it's different from C# and it doesn't have to be. We have to choose a default for each attribute - are you in a C# context or a text context. Every time an attribute is different, or unique, that's something that consumer have to learn about instead of just writing normal C#.

I don't think that shorter necessarily means better.

arivoir commented 5 years ago

My reluctance is that it's different from C# and it doesn't have to be. We have to choose a default for each attribute - are you in a C# context or a text context. Every time an attribute is different, or unique, that's something that consumer have to learn about instead of just writing normal C#

What's the semantical difference between assigning a string directly or having to use a "@" plus parentheses?

GridLinesColor="@("#FFD0D7E5")" vs GridLinesColor="#FFD0D7E5"

Sorry I don't get your point. What you say it is different from c# is exactly what Blazor is doing right now with string class to avoid gross syntax, as you described. You are contradicting your own argument.

I don't think that shorter necessarily means better.

I agree, but in this case I think the extra "@" and "()" doesn't give any advantage to the users, all the contrary it makes the razor language boilerplate unnecessarily.

rynowak commented 5 years ago

Sorry I don't get your point. What you say it is different from c# is exactly what Blazor is doing right now with string class to avoid gross syntax,

Right. Strings are special cased because the alternative is awful.

If you want to use some other type that isn't a string, there is no extra ceremony required.

@{
   Color color = ....
}

<SomeComponent Color="color" />
<SomeComponent Color="new Color("#FFFFFF")" />
arivoir commented 5 years ago

Right. Strings are special cased because the alternative is awful.

We both agree about this.

<SomeComponent Color="new Color("#FFFFFF")" />

I think I'm starting to get your point. I thought the above didn't work unless an "@" would be used. I imagined that inserting a peace of c# inside html would require the "@" (and the lack of intelli-sense coloring for some cases made me confuse).

Anyway I wonder if requiring the "@" to change context wouldn't be a better alternative to actual implementation. It would be more intuitive, the string case wouldn't be different from the rest, and it would allow enums and custom classes to be more easily specified. What do you think?

rynowak commented 5 years ago

Anyway I wonder if requiring the "@" to change context wouldn't be a better alternative to actual implementation. It would be more intuitive, the string case wouldn't be different from the rest, and it would allow enums and custom classes to be more easily specified. What do you think?

We considered and rejected this because:

  1. Existing ASP.NET Core (TagHelpers) already put you in a C# context
  2. Requiring @ to change to C# context breaks compound expressions like 3 + 5
arivoir commented 5 years ago

We're on the same page now. Thanks for taking time to explain this to me. I'll try to catch up with TagHelpers https://docs.microsoft.com/en-us/aspnet/core/mvc/views/tag-helpers/intro?view=aspnetcore-2.2

arivoir commented 5 years ago

Does TagHelpers work in Blazor?

rynowak commented 5 years ago

No, tag helpers are incompatible at a fundamental level. TagHelpers output text, Blazor is like an instruction language for markup.

arivoir commented 5 years ago

I'm having trouble to understand this argument

  1. Existing ASP.NET Core (TagHelpers) already put you in a C# context

I wonder if this applies to Blazor anyhow.

Regarding the other argument

  1. Requiring @ to change to C# context breaks compound expressions like 3 + 5

I would feel rather natural to need to use the "@" in this case.

MyProperty="@(3+5)"

rynowak commented 5 years ago

I wonder if this applies to Blazor anyhow.

We want to be consistent and avoid two technologies that look the same with totally different semantics.

arivoir commented 5 years ago

We want to be consistent and avoid two technologies that look the same with totally different semantics.

I can understand this. But anyway I don't get what does this mean "TagHelpers already put you in a c# context" and how is this related to the fact that the component attributes are actually c# code even if the "@" is not specified?

rynowak commented 5 years ago

They are consistent today. Like I said, we considered a change and decided to do the same thing tag helpers do.

arivoir commented 5 years ago

Whether or not an @ is required inside a component attribute is a function of the type of the declaring parameter. String-typed parameters are text by default because otherwise the syntax would be gross.

<MyComponent 
  BoolParameter="<this is C#>" 
  StringParameter="<this is text>" 
  AnotherStringParameter="@<now this is C#>"/>

This was the post that clarified to me how Blazor is working.

I've had some time to think on this, and still think there could be a way to use TypeConverter. You said there is a function that determines whether the parameter type is a string and in that case assigns the string directly into the property, otherwise the attribute value is c#.

My point is the function that determined the kind of content inside the attribute, could use https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.typeconverter.canconvertfrom?view=netframework-4.8 with typeof(string) to determine whether the attribute value is a plain string of c#. Wouldn't that work?

mkArtakMSFT commented 4 years ago

Thanks for contacting us. After some further discussion regarding this we believe this is not something we plan to do.