Route events based on delegate target, not component ID

[SteveSandersonMS]

This fixes multiple issues and potential areas of confusion related to event routing (in effect, the scenarios where you have to call StateHasChanged or not).

Previously, if one component passed an event handler (e.g., an Action, Func<Task>, MulticastDelegate, etc.) through to another component, either as a [Parameter] or simply by including it in some ChildContent, then when the event was fired it would be routed not to the component that owns the event handler, but the one that rendered it into the tree.

Example

MyButton.cshtml:

<button onclick=@OnClick>Click me<button>
@functions {
    [Parameter] Action OnClick { get; set; }
}

Consumer:

<MyButton OnClick=@SomeMethod /> or <MyButton OnClick=@(() => { clickCount++; }) />

Clicks: @clickCount

@functions {
    int clickCount = 0;

    void SomeMethod()
    {
        clickCount++;
    }
}

You'd expect that clicking the button would update the click count, but it wouldn't. You'd have to fix it by calling StateHasChanged manually in the consumer's SomeMethod or the lambda. This was annoying and confusing.

Example 2:

PassthroughComponent.cshtml:

@ChildContent

@functions {
    [Parameter] RenderFragment ChildContent { get; set; }
}

Consumer.cshtml:

<PassthroughComponent>
    <button OnClick=@(() => { clickCount++; })>Click me</button>
</PassthroughComponent>

Clicks: @clickCount

@functions {
    int clickCount = 0;
}

Again, clicking the button would seem to have no effect, unless you manually put StateHasChanged into the lambda.

Fix

We now route events to the component matching eventHandlerDelegate.Target, not the component that inserted the delegate into the render tree. This fixes both of the common cases above, and works with event handlers expressed as both methods and lambdas.

This also simplifies cases around binding. Components that expose custom bindables can now trigger updates in their consumers easily.

New API

In the case where a custom component wants to trigger a supplied event handler delegate indirectly (e.g., inside one of its own methods), there's now a public static API:

ComponentEvents.InvokeEventHandler(someDelegate, eventArgs)

This works with delegates that are Action, Action<T>, Func<Task>, Func<T, Task> (the fast paths), or arbitrary MulticastDelgate (slower path). It automatically causes the recipient to trigger its own re-rendering logic, including responses to async tasks (i.e., if it's a Func<Task>, then the recipient renders once synchronously, then again asynchronously after the task, just like normal event handlers).

Breaking change

There's one scenario where this may break something people were already doing, i.e., relying on the older strange behavior. Example:

<button onclick=@SomeOtherObject.SomeMethod>Click me</button>

Previously, this would run SomeOtherObject.SomeMethod and then re-render the component. That might be useful if this method updated some global state that you read back during rendering.

Now it will no longer re-render the component, because SomeMethod isn't on that component. If you actually do want to re-render the component in this case, the solution is to ensure you're actually calling one of its methods or lambdas, e.g.:

<button onclick=@(() => SomeOtherObject.SomeMethod())>Click me</button>

Alternatively, if you're actually updating some global state container, it's better still to have the state container raise some event that triggers all the necessary UI updates on all components, not just the one that raised the action.

Although it's a slight drawback to have this breaking change, I still think this is the right thing to do. The new rules about when events update components are much simpler and easier to reason about (i.e., we re-render the component that owns the event handler method/lambda, regardless of who passed it where).

[rynowak]

I'm assuming these are removed as unused (same with platform)

[SteveSandersonMS]

Correct

[rynowak]

I'm assuming that a lot of this got simpler because we no longer track the componentId associated with an eventHandler on the JS side. If this isn't right let me know 😆

[SteveSandersonMS]

Exactly

[rynowak]

IsSameTarget? I would expect a method named like this to throw

[SteveSandersonMS]

That's reasonable. I'll change it.

[rynowak]

I'm guessing that this validation is lost now that we've generalized this. I don't have a big concern about it because I haven't seen anyone complain about this error message.

[SteveSandersonMS]

This error is no longer applicable as a concept.

[rynowak]

Sort order is wrong

[rynowak]

Is this an intentional change or did we create another gotcha here?

Based on what I can think of there's now a semantic difference between a capturing lambda and a method-group-to-delegate conversion in this case.

[SteveSandersonMS]

This is the "breaking change" mentioned in the PR description.

There's now a semantic difference between passing delegates (e.g., lambdas) on the component type versus passing delegates on other types. There's no difference between lambdas versus methodgroup-to-delegate on the same component type. Please let me know if you think I'm incorrect or if you disagree with this design.

[SteveSandersonMS]

No wait, there is a problem. Hmm... investigating options.

[SteveSandersonMS]

OK, turns out this is fundamentally flawed, and after some consideration, I don't have a sufficiently simple and robust plan to salvage the concept. I should have spotted this earlier, especially given Ryan's hints about how capturing lambdas might cause issues.

I had been testing with lambdas that capture this (which is why I thought it worked), but not capturing anything else. As soon as you capture something other than this, then delegate.Target has to start referring to the generated closure data class (of course!). That breaks this whole approach, because it means there's a semantic difference between onclick=@(() => DoSomething()) and onclick=@(() => DoSomething(someLoopVariable)) (in that the latter will no longer trigger an automatic re-render). This semantic difference is not acceptable.

This whole idea of relying on the original target getting stamped on the delegate on creation seemed like a great solution because it survives the delegate getting passed through properties on any number of intermediate components. But if delegate.Target doesn't refer to the original target, by the time it's been passed through the first layer, we've totally lost track of which component instance created it.

Frustratingly the exact data I need is right there on the public <>4__this property on delegate.Target, but since there's no supported API for reading that, I don't think we can take a dependency on reading it directly.

Possible approach to salvage this:

• We could change the codegen for events on elements so that instead of generating GetEventHandlerValue(yourSource), we generate GetEventHandlerValue(this, yourSource)
• Also change the codegen for events on components so that instead of generating new Func<Task>(yourSource) (etc., for all the supported delegate types), we'd generate a similar call to GetEventHandlerValue passing this and yourSource.
• In GetEventHandlerValue, we stamp the this value onto the constructed delegate by writing it to some WeakDictionary<MulticastDelegate, object> (except if there's already an entry for this delegate).

Apart from the complexity, what I dislike about this is that, if you're writing a RenderFragment in plain C#, you wouldn't realise that your event handler values have to be wrapped in GetEventHandlerValue(this, value), so your event handlers would just appear not to work properly.

Other possible approaches:

• Declare that we never trigger re-rendering automatically after events. Developers always have to put StateHasChanged at the end of event handlers manually.
• Or, make it a compiler feature, whereby the runtime assumes you will call StateHasChanged manually, and have the compiler detect usages of lambdas and insert StateHasChanged(); at the end (likewise, detect use of methodgroups, and replace them with lambdas that end with StateHasChanged();). This would not preserve the existing behavior of re-rendering both synchronously and asynchronously after any returned task - it would just do the latter.

@rynowak Do you agree with this assessment? Any other ideas? I'm not expecting anything in this area to make it in for 0.7.0 any more.

[SteveSandersonMS]

Another possibility is that we declare some new "event handler" types (say, UIEventHandler and UIEventHandler<T>) and say that these are the correct types for all event-handler parameters on components, e.g., for MyButton, you'd have

[Parameter] UIEventHandler<UIClickEventArgs> OnMySpecialClick { get; set; }

Then we change the codegen both for regular elements and for components so that event attributes produce some value like UIEventHandler.Create(this, yourSourceCode). We overload that method to take Action/Action<T>/Func<Task>/Func<T,Task>/UIEventHandler/UIEventHandler<U>. If you give it an UIEventHandler/UIEventHandler<U>, it just returns the same instance back, but for all the others it constructs an UIEventHandler/UIEventHandler<T> that tracks this as well as the delegate.

One nice benefit of this is that component authors no longer have to decide between Action and Func<Task> for their event parameters. They would use UIEventHandler and it would transparently work with both sync and async handler delegates. However, if the component author chooses to use Action (etc) directly then they won't have any way to trigger the consumer re-render.

Components can pass UIEventHandler parameters to their children without it interfering with the this association. When the framework sees an UIEventHandler instance as the object associated with an event being invoked, it can go through the same process that this PR does to check if this is IHandleEvent and calls its notification APIs if so.

Developers writing a RenderFragment in plain C# would have to understand that you need to declare event handlers like:

builder.AddAttribute(0, "onclick", new EventHandler(this, () => { ... }));

That's not 100% ideal but writing RenderFragment in plain C# is pretty advanced anyway.

Even if this design is viable, I don't expect it to make it in for 0.7.0.

[SteveSandersonMS]

Further idea (noting to myself for future reference): we may be able to avoid declaring any UIEventHandler type, and instead codegenning something like CreateEventHandler(StateHasChanged, yourSource), where CreateEventHandler is some static method like:

static Func<Task> CreateEventHandler(Action postEventCallback, Action eventHandler)
static Func<T, Task> CreateEventHandler<T>(Action postEventCallback, Action<T> eventHandler)
static Func<Task> CreateEventHandler(Action postEventCallback, Func<Task> eventHandler)
static Func<T, Task> CreateEventHandler<T>(Action postEventCallback, Func<T,  Task> eventHandler)

each of which would do something like:

if (eventHandler.Target == typeof(TypeContainingCreateEventHandler))
{
    return eventHandler; // Pass through existing wrapped delegates without extra wrapping
}
else
{
    // Create a new delegate whose Target == typeof(TypeContainingCreateEventHandler)
    // as a signal not to wrap it further later
    return () =>
    {
        var result = eventHandler();
        postEventCallback();
        return result.ContinueWith(() => postEventCallback());
    };
}

Then, the recipient gets a regular Func<Task> they call in the usual way without needing any equivalent to the ComponentEvents.Invoke from this original PR, and it's safe to pass through into further levels of CreateEventHandler because it won't be wrapped further (except if you explicitly call it from your own lambda/method, in which case the extra wrapping is desirable anyway).

People writing RenderFragment in C# have the choice to call CreateEventHandler or to just pass a manually-written Func<Task> which may or may not end with StateHasChanged().

This also completely eliminates the notion of IHandleEvent, since it becomes an aspect of how we codegen event handlers for Razor components in the compiler.

Still need to think through exactly what allocations this leads to. If the methodgroup-to-delegate on StateHasChanged allocates every time, we could manually generate a field to cache this on the component class.

[SteveSandersonMS]

Closing this because we're not concluded on the design yet. @rynowak and I are still discussing where we're going with bind and event handling.