Call RenderTreeBuilder.AddComponentParameter#8286
Conversation
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
The general concept seems good to me.
|
dotnet/aspnetcore#46562 was just merged, which adds the |
jjonescz
changed the title
RenderTreeBuilder.AddComponentParameter
| public static readonly RazorLanguageVersion Version_7_0 = new RazorLanguageVersion(7, 0); | ||
|
|
||
| public static readonly RazorLanguageVersion Latest = Version_7_0; | ||
| public static readonly RazorLanguageVersion Version_8_0 = new RazorLanguageVersion(8, 0); |
There was a problem hiding this comment.
Do we know whether the default is latest or 7? If the latter, where do we need to update the default?
There was a problem hiding this comment.
The default is latest as far as I can see in Razor (judging by C# references to RazorLanguageVersion and MSBuild property RazorLangVerision). We might need to update SDK, though? https://github.com/dotnet/sdk/blob/main/src/RazorSdk/Targets/Sdk.Razor.CurrentVersion.targets
There was a problem hiding this comment.
Yeah, looks like it: https://github.com/dotnet/sdk/blob/main/src/RazorSdk/Targets/Sdk.Razor.CurrentVersion.targets#L46-L91. Let's file a bug to track that work.
|
@chsienki @dotnet/razor-compiler for the second review |
| public static global::Test.MyComponent<T> CreateMyComponent_0<T>(global::Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder, int seq, int __seq0, global::System.Boolean __arg0, int __seq1, global::System.String __arg1, int __seq2, global::System.Delegate __arg2, int __seq3, global::System.Object __arg3, int __seq4, global::Test.MyClass<T> __arg4, int __seq5, global::Microsoft.AspNetCore.Components.EventCallback<global::Test.MyClass<T>> __arg5) | ||
| { | ||
| __builder.OpenComponent<global::Test.MyComponent<T>>(seq); | ||
| __builder.AddAttribute(__seq0, "BoolParameter", __arg0); |
There was a problem hiding this comment.
Whats the reason for special casing bool in general?
There was a problem hiding this comment.
Whats the reason for special casing bool in general?
Same question from me. Keeping the old behavior for bool parameters means the bug still exists if the parameter value's type defines an implicit conversion operator to bool.
There was a problem hiding this comment.
Whats the reason for special casing
boolin general?
That was meant as an optimization, because AddAttribute(..., bool) handles boxing of the bool more efficiently (it uses pre-computed BoxedTrue and BoxedFalse constants). Without special handling it, AddAttribute(..., (object)BoolArg) gets called - boxing true or false at the callsite.
Keeping the old behavior for
boolparameters means the bug still exists if the parameter value's type defines an implicit conversion operator tobool.
That's a good catch, thanks. I will remove the special casing to fix this.
|
@333fred I removed |
4 participants
Fixes dotnet/aspnetcore#18042.
Resolves #8258.
Calls new method
RenderTreeBuilder.AddComponentParameteradded in dotnet/aspnetcore#46562.Old version: casting component attributes to
objectGenerates
builder.AddAttribute(seq, name, (object)value);calls for component parameters. This avoids problems wherevaluehas implicit conversion to another primitive type (likestring). Boxing is not a problem, it is actually required for component parameters, so this should allow the runtime to simplify their logic inRenderTreeBuilder.Boolean parameters are excluded from the casting because they are more efficiently boxed inside
RenderTreeBuilder(reusing boxed true and false values).If method
RenderTreeBuilder.AddComponentParameteris added in dotnet/aspnetcore#46562, changes in this PR can be revisited to call that overload instead. (Or we can do that immediately, I just wanted to investigate what changes are needed.)CC: @MackinnonBuck