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

Improve Blazor CascadingTypeParameter support - Current compiler cannot handle common scenarios. #10757

Open
1 task done
audacode opened this issue Aug 17, 2024 · 4 comments
Assignees
Labels
area-blazor area-compiler Umbrella for all compiler issues untriaged
Milestone

Comments

@audacode
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

I expected the CascadingTypeParameter feature to work in the most common and obvious scenarios, but it does not. If the cascaded type was a string, it should be possible to use <Widget Action="item => Console.WriteLine(item)" /> instead of <Widget Action="(string item) => Console.WriteLine(item)" />. If you were passing in a parameter, it should be possible to do this <Widget Item="Hello" /> instead of needing to do this <Widget Item="@("Hello")" />. The CascadingTypeParameter feature was intended to resolve this type of issue but needs more work.

Describe the solution you'd like

Imagine this repeater:

@attribute [CascadingTypeParameter(nameof(T))]
@typeparam T

@foreach (var item in Items)
{
    @ChildContent(item)
}
@code {
    [Parameter] public RenderFragment<T> ChildContent { get; set; }
    [Parameter] public IEnumerable<T> Items { get; set;}
}

Then this Widget component intended to be used inside the repeater:

@typeparam T
Type is @typeof(T)
@code {
    [Parameter] public EventCallback<T> Action { get; set; }
    [Parameter] public T? Item { get; set; }
}

Warning: Do not overthink this widget as it is not intended to do anything sensible, other than demonstrate compiler limitations.

Now imagine we use the component like such:

<Repeater Items="items">
    <Widget Action="(string i) => Console.WriteLine(i)" />
</Repeater>
@code {
List<string> items = ["David", "Fred", "Jimmy", "Banjo"];
}

The above code works, and <Widget T="string" Action="i => Console.WriteLine(i)" /> also works. However, it is completely reasonable to expect to be able to use the following syntax:

<Widget Action="i => Console.WriteLine(i)" />

Another example of the compiler's mediocre compilation process is the following example. Let us say we write this code:

<Widget Item=@("Hello") />

That works, but because T is a string, for consistency with the way the rest of blazor works, we would expect this to work (noting that it does work if the underlying type was a string instead of a type parameter, where the parameter was a string) :

<Widget Item="Hello" />

However, this does not work (it looks for a member called Hello, which is what blazor would normally do for anything other than a string. Even this does NOT work: <Widget T="string" Item="Hello" />.

@danroth27 and @SteveSandersonMS I had thought we were further along than this once we got to .NET 6 and added the CascadingTypeParameter feature. It is hard, not being on the compiler team, to understand why when an outer component uses CascadingTypeParameter, that that isn't the same as directly passing in the type using the old-school T="string" approach. But I do feel the current expectation means almost everyone will hit this issue and get stuck/frustrated.

Is there a concrete reason why the situation cannot be improved for .NET 9 or even .NET 10 if that is needed.

Additional context

No response

@javiercn javiercn transferred this issue from dotnet/aspnetcore Aug 17, 2024
@davidwengier davidwengier added the area-compiler Umbrella for all compiler issues label Aug 18, 2024
@audacode
Copy link
Author

audacode commented Aug 18, 2024

Hi @davidwengier nice train simulator! I am Melbourne based as well.

If it helps, I notice that if the Widget.razor file uses an Action like this:

    [Parameter] public Action<T> Action { get; set; }

Then you can use this code without specifying the type of i:

    <Widget Action="i => Console.WriteLine(i)" />

But if it is specified using EventCallback like this:

    [Parameter] public EventCallback<T> Action { get; set; }

Then you you must specify the type of i either like this <Widget Action="(string i) => Console.WriteLine(i)" /> or this <Widget T="string" Action="i => Console.WriteLine(i)" />

I have not looked under the covers, but note what WORKS and DOES NOT work below:

        EventCallbackFactory factory = new();
        // This works
        EventCallback<string> f1 = factory.Create<string>(null, i => Console.WriteLine(i));
        // This works
        EventCallback<string> f2 = factory.Create(null, (string i) => Console.WriteLine(i));
        // This DOT NOT work
        EventCallback<string> f3 = factory.Create(null, i => Console.WriteLine(i));

Thus, my assumption is that the Razor compiler is not emitting this as a call to .Create<TCascadedType> and is instead using ```.Create`` causing the issue.

I am guessing if the compiler emits the call to the cascaded type parameter, such as to .Create<string> that would resolve the issue? I have to assume there is a way of getting the razor compiler to handle this situation. We want developers to be using EventCallback rather than Action because it plays better with the way Blazor interacts with StateHasChanged when the callback happens.

@jjonescz
Copy link
Member

Thanks for writing this up. If I'm reading it correctly, you are reporting two separate issues:

  1. CascadingTypeParameter and EventCallback don't play nicely in all cases. I haven't investigated deeply, so this might be wrong, but as you said, it looks like the razor compiler should emit EventCallbackFactory.Create<T> if there is type inference involved - and where Action<T> works, EventCallback<T> should work similarly.
  2. Passing a string literal directly to a generic component parameter which we know has type string doesn't work. This is not related to CascadingTypeParameter, e.g., the following doesn't work either:
    <Widget T="string" Item="Hello" />
    Whereas if the Widget wasn't generic, i.e., the Item parameter type was simply string, that would work.
    Changing that would be a breaking change though - if users are today relying on this behavior and passing a variable, it would suddenly start to be interpreted as a string literal.

@chsienki chsienki added this to the Backlog milestone Aug 22, 2024
@iphdav
Copy link

iphdav commented Aug 24, 2024

@jjonescz Yes I agree with your summary of point 1 that "if there is type inference involved - and where Action works, EventCallback should work similarly."

I think this should be a simple change and would ideally get into .NET 9. I was surprised this did not work out of the box. I wasted a lot of time on it because I believed it must have been me doing something stupid instead of the compiler not supporting EventCallback in that particular and common case.

Regarding point 2: Thank you for clarification and I agree with that it would be a breaking change so will need to be done through good documentation rather than a change (and I should not have included point 2 in the same bug/issue report - sorry).

@jjonescz
Copy link
Member

Hm, I've investigated the EventCallback<T> problem more and I don't think it's easy to fix.

The razor compiler doesn't know the T at compile-time, it very deliberately relies on the C# compiler to do the type inference.

For example, for an Action<T> parameter we generate this C#:

CreateWidget(i => Console.WriteLine(i));

void CreateWidget<T>(Action<T> a) { }

but for EventCallback<T> we need to call EventCallbackFactory (it has multiple overloads to support scenarios where user passes Func<Task> for example which is then converted to the EventCallback<T> etc):

CreateWidget(EventCallback.Factory.Create(i => Console.WriteLine(i)); // error: the factory creates EventCallback instead of EventCallback<T>

void CreateWidget<T>(EventCallback<T> e) { }

If runtime provided EventCallbackFactory.CreateGeneric which would only have overloads returning EventCallback<T> we could use that (when we know the parameter type is the generic EventCallback<T>) and it would work, I think. Otherwise, I don't see an easy solution we could do in the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor area-compiler Umbrella for all compiler issues untriaged
Projects
None yet
Development

No branches or pull requests

5 participants