Blazor API review changes: CompilerServices #11911
Conversation
rynowak
force-pushed
the
rynowak/compiler-api-review
branch
from
3dbb13e to
a376ef4
Compare
|
See issue for rationale |
src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs
Show resolved
Hide resolved
| /// <param name="sequence">An integer that represents the position of the instruction in the source code.</param> | ||
| /// <param name="name">The name of the attribute.</param> | ||
| /// <param name="value">The value of the attribute.</param> | ||
| public void AddAttribute(int sequence, string name, Action<UIEventArgs> value) |
There was a problem hiding this comment.
These are unused except for some unit tests. It's still possible to attach arbitrary delegates to the RTB - these methods are here to support method-group-to-delegate conversion and aren't really needed anymore.
There was a problem hiding this comment.
It's still possible to attach arbitrary delegates to the RTB
But there's no use case for it, is there? Is this what we're discussing above with "should we remove the Action/Func<T> overloads of AddAttribute, or is this something else?
There was a problem hiding this comment.
But there's no use case for it, is there?
The only use case for it would be making it nicer for someone writing RTB code by hand. Let's look at this again during the RTB API review.
rynowak
force-pushed
the
rynowak/compiler-api-review
branch
from
a376ef4 to
f1bfdc4
Compare
Fixes: #11907 These are the changes from the compiler-related API review that are safe to make (these methods are not used). I'll follow up with `BindMethods.GetValue` separately because that's used by the compiler.
rynowak
force-pushed
the
rynowak/compiler-api-review
branch
from
f1bfdc4 to
2d8437c
Compare
| { | ||
| HandlerWasCalled = true; | ||
| } | ||
| }"); |
There was a problem hiding this comment.
@SteveSandersonMS - speakup if you have concerns. I'm specifically removing support for things like this (not using directive attributes). Should we do this with Action and Func<Task> as well?
There was a problem hiding this comment.
I expect this is fine, including removing the Action/Func<Task> variants too. The ideal, if we have time for it, would be issuing a compile-time warning for people who put attributes named onX with delegate-type values when they fail to use the directive attribute.
I just checked that our extensibility for event tag helpers works nicely (it does), so people are free to define their own custom event type names and values. This is essential because the HTML spec continues to evolve, and new or nonstandard event names will keep appearing.
Given that the tag helper extensibility works well, there's no use case for people trying to define events handlers any other way. The only risk is that they don't know about how to do it, which is where a compiler warning (perhaps with a fwlink to docs) would be ideal. If we don't have time to add the warning now, it could be added in the future.
There was a problem hiding this comment.
If we don't have time to add the warning now, it could be added in the future
I'm not certain how this would work in practice. We don't know in the Razor compiler what kind of value you're passing in when you set an attribute.
src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs
Show resolved
Hide resolved
| { | ||
| // Arrange | ||
| var builder = new RenderTreeBuilder(new TestRenderer()); | ||
|
|
||
| // Act/Assert | ||
| Assert.Throws<InvalidOperationException>(() => | ||
| { | ||
| builder.AddAttribute(0, "name", eventInfo => { }); | ||
| builder.AddAttribute(0, "name", new Action<UIEventArgs>(eventInfo => { })); |
There was a problem hiding this comment.
Could you clarify why this change is here? The test name only talks about delegates in general, not specific types of delegates. So why wasn't the previous code sufficient to represent that?
There was a problem hiding this comment.
This is needed to make the type conversion work. I tried to keep the semantics of what's being tested the same.
ghost
deleted the
Fixes: #11907
These are the changes from the compiler-related API review that are safe
to make (these methods are not used).
I'll follow up with
BindMethods.GetValueseparately because that'sused by the compiler.