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

TypeConverter for parameters #8493

Closed
arivoir opened this issue Mar 13, 2019 · 34 comments
Closed

TypeConverter for parameters #8493

arivoir opened this issue Mar 13, 2019 · 34 comments
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. Status: Resolved
Milestone

Comments

@arivoir
Copy link

arivoir commented Mar 13, 2019

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.

@muratg muratg added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 14, 2019
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components question enhancement This issue represents an ask for new feature or an enhancement to an existing one PRI: 2 - Preferred and removed question labels Mar 25, 2019
@mkArtakMSFT
Copy link
Member

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.

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Mar 29, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
rynowak pushed a commit that referenced this issue Jun 1, 2019
Fixes: #8493
Fixes: #9632
Fixes: #9339
Fixes: #8385
Fixes: 10077

This fix adds support for type converters as well as a few other minor
things we were missing from binding. The key thing about supporting
conversions is that we new can support arbitrary types with `@bind`.
This means you can use it with generics, which is something many users
have tried.

Along with type converters we get Guid and TimeSpan from the BCL. The
BCL also includes converters for types we're less interested in like
`short`.
rynowak pushed a commit that referenced this issue Jun 3, 2019
Fixes: #8493
Fixes: #9632
Fixes: #9339
Fixes: #8385
Fixes: 10077

This fix adds support for type converters as well as a few other minor
things we were missing from binding. The key thing about supporting
conversions is that we new can support arbitrary types with `@bind`.
This means you can use it with generics, which is something many users
have tried.

Along with type converters we get Guid and TimeSpan from the BCL. The
BCL also includes converters for types we're less interested in like
`short`.
rynowak added a commit that referenced this issue Jun 5, 2019
* Add support for TypeConverter

Fixes: #8493
Fixes: #9632
Fixes: #9339
Fixes: #8385
Fixes: 10077

This fix adds support for type converters as well as a few other minor
things we were missing from binding. The key thing about supporting
conversions is that we new can support arbitrary types with `@bind`.
This means you can use it with generics, which is something many users
have tried.

Along with type converters we get Guid and TimeSpan from the BCL. The
BCL also includes converters for types we're less interested in like
`short`.

* Use correct NumberStyles

* Fix culture

* Core check
@rynowak
Copy link
Member

rynowak commented Jun 5, 2019

This has been added in preview 7 - #10730
Note that preview 6 is the next release so you will have to wait a few weeks.

@rynowak rynowak modified the milestones: Backlog, 3.0.0-preview7 Jun 5, 2019
@rynowak rynowak self-assigned this Jun 5, 2019
@arivoir
Copy link
Author

arivoir commented Jun 5, 2019

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
Copy link
Member

rynowak commented Jun 5, 2019

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

@arivoir
Copy link
Author

arivoir commented Jun 5, 2019

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
Copy link
Member

rynowak commented Jun 6, 2019

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
Copy link
Author

arivoir commented Jun 6, 2019

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
Copy link
Member

rynowak commented Jun 7, 2019

Yes, the type converter is used in parsing.

@arivoir
Copy link
Author

arivoir commented Aug 2, 2019

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

@arivoir
Copy link
Author

arivoir commented Aug 2, 2019

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
Copy link
Member

rynowak commented Aug 2, 2019

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
Copy link
Author

arivoir commented Aug 2, 2019

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
Copy link
Member

rynowak commented Aug 2, 2019

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
Copy link
Author

arivoir commented Aug 2, 2019

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
Copy link
Member

rynowak commented Aug 2, 2019

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
Copy link
Author

arivoir commented Aug 5, 2019

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
Copy link
Member

rynowak commented Aug 5, 2019

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
Copy link
Author

arivoir commented Aug 5, 2019

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
Copy link
Member

rynowak commented Aug 5, 2019

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
Copy link
Author

arivoir commented Aug 5, 2019

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
Copy link
Author

arivoir commented Aug 5, 2019

Does TagHelpers work in Blazor?

@rynowak
Copy link
Member

rynowak commented Aug 5, 2019

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

@arivoir
Copy link
Author

arivoir commented Aug 5, 2019

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
Copy link
Member

rynowak commented Aug 5, 2019

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
Copy link
Author

arivoir commented Aug 5, 2019

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
Copy link
Member

rynowak commented Aug 5, 2019

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

@arivoir
Copy link
Author

arivoir commented Sep 30, 2019

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
Copy link
Member

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

@mkArtakMSFT mkArtakMSFT added the ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. label Jan 23, 2020
@ghost ghost added the Status: Resolved label Jan 23, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. Status: Resolved
Projects
None yet
Development

No branches or pull requests

5 participants