dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.51k stars 10.04k forks source link

Design proposal: Multiple same-named event handlers #39815

Closed SteveSandersonMS closed 2 years ago

SteveSandersonMS commented 2 years ago

Summary

Blazor should allow HTML elements and components to accept multiple event handlers for a single event name.

Motivation and goals

This was requested at https://github.com/dotnet/aspnetcore/issues/14365 and is well upvoted. It deals with the following pain points:

Really, the case with @bind is the important one, as it's fairly common and existing workarounds (such as custom property setters) are very messy and don't even work if you're trying to perform an async action.

In scope

Out of scope

Risks / unknowns

  1. Given <input @onchange="MyAsyncLogic" @bind="SomeValue">, developers might think that - because MyAsyncLogic comes first - we'd wait arbitrarily long for its async task to complete before updating SomeValue. This is not the case. Conceptually, multiple event handlers will run in parallel, even though you can control the order in which they start. This is because:
    • It's necessary to satisfy the principle of "each event handler behaves independently", which in turn is necessary to make it practical to reason about.
    • It would completely wreck the behavior of @bind if there were asynchronous delays before it. Blazor Server is extremely careful about this to ensure that keystrokes are never lost and the UI doesn't revert to a prior state when performing unrelated re-renders.
  2. It might complicate things in the future if we decide to allow JS-style event mutation, e.g., for a handler to cancel the event. We could probably at least do whatever JS does.
  3. Developers might think that, because you can do @onclick=A @onclick=B that you should also be able to do @ref=A @ref=B or @key=A @key=B or SomeComponentParam=A SomeComponentParam=B. None of those other cases are allowed, mostly because they don't have clear meanings and nobody wants them.

Examples

The most classic use case is to perform some action before or after binding.

<input @bind="searchText" @onchange="PerformSearch" />

@code {
    string searchText;

    async Task PerformSearch()
    {
        // ... use the updated searchText value ...
    }
}

Without this feature, you have to do some really nasty stuff to achieve the above, e.g., binding to a property with a custom setter and then discard the Task.

A more complex variant of the above is with component binding:

<input @onchange="BeforeValueChanged" @bind="BoundValue" @onchange="AfterValueChanged" />

@code {
    string BoundValue
    {
        get => Value;
        set => ValueChanged.InvokeAsync(value);
    }

    [Parameter] public string Value { get; set; }
    [Parameter] public EventCallback<string> ValueChanged { get; set; }

    [Parameter] public EventCallback<ChangeEventArgs> BeforeValueChanged { get; set; }
    [Parameter] public EventCallback<ChangeEventArgs> AfterValueChanged { get; set; }
}

Note that this doesn't solve the problem that we're discarding the Task returned by ValueChanged.InvokeAsync, but it does mean that BeforeValueChanged and AfterValueChanged can have correct async behaviors and not discard their tasks.

Detailed design [OPTIONAL - only read if you don't have questions/feedback about the above]

I think we already have consensus that the feature is desirable, so I'm going to sketch some design thoughts. These could change if we end up changing any of the scope and goals.

Representing multiple event handlers

When we have <button @onclick=A @onclick=B>, how is the set of event handlers stored internally? This is a key question that underpins most of the possible implementation choices. I can think of three main choices:

A. Multiple values with the same name. For example, there would be two RenderTreeFrame entries both with the AttributeName value onclick.

B. Multiple values with different names. For example, there would be two RenderTreeFrame entries, and the compiler would produce mangled names like onclick and onclick.1.

C. Single value encapsulating all handlers. For example, there would continue to be a single RenderTreeFrame entry, and its value would represent all the handlers.

However, the fact that we also want to support this on component parameters (not just HTML elements) with CaptureUnmatchedValues imposes further restrictions that eliminates some options. Consider <MyComponent @onclick=A @onclick=B>. The component is going to receive an IDictionary<string, object> representing the attributes. So:

Between C1 and C2, it's fairly clear that C2 is more realistic. So let's imagine that one in more detail.

Introducing MulticastEventCallback

Invent a new kind of event callback that represents an ordered set of mutually-compatible event callbacks.

Sidenote on naming

We could call it:

They need to be mutually compatible in the sense that there has to be a single most-specific eventargs type, since the client is only calling the event once and passing a single eventarg. So, the set can include EventCallback, EventCallback<ChangeEventArgs> and EventCallback<ChangeEventArgsSubclass> (and the client would be asked to send ChangeEventArgsSubclass). But it can't include EventCallback<ChangeEventArgs> and EventCallback<MouseEventArgs>, as there's no most-specific type.

So it's not an arbitrary set, which leads me to prefer the name MulticastEventCallback over the group/collection names. It hints at the "single input, multiple output" nature of this concept.

Generated code

Given <button @onclick=A @onclick=B>, we'd emit:

builder.OpenElement(0, "button");
builder.AddAttribute(1, "onclick", new MulticastEventCallback(
    EventCallback.Factory.Create<MouseEventArgs>(this, A).AsUntyped(),
    EventCallback.Factory.Create<MouseEventArgs>(this, B).AsUntyped()));
builder.CloseElement();

... where we also define:

// It's a struct because, even though it always gets boxed when stored on a RenderTreeFrame, it makes the storage inside Renderer's dictionary cheaper (or at least better memory locality).
public readonly struct MulticastEventCallback
{
    public MulticastEventCallback(params[] EventCallback callbacks) { ... }

    // Of course we could also special case overloads for one param, two params, etc., if it turns out to reduce the allocations on calling it. Internally we might have a fixed number of EventCallback fields along with an array field that's only used if there are too many.
}

Since the generated code still uses existing EventCallback.Factory.Create<T> to produce the EventCallback values, all the existing logic around overload selection and generic type inference will continue to work.

Another benefit from MulticastEventCallback being public API is that component authors who have complex custom requirements can build arbitrary logic around it. For example, they can have arbitrary rules for CaptureUnmatchedValues appending, prepending, or overwriting other handlers if they look inside the supplied dictionary and construct their own MulticastEventCallback value to use as an @onevent=... value.

Notes for me:

mkArtakMSFT commented 2 years ago

Thanks for putting this together, @SteveSandersonMS. I was a bit hesitant at first regarding the auto-re-rendering after each handler completes, but after some more thoughts I believe what you have is great.

pranavkm commented 2 years ago

Given <input @bind=A @onchange=B>, the existing compiler emits diagnostic RZ10008 and fails compilation. We will remove this diagnostic completely.

Presumably this needs to be behind a LangVersion=7.0 switch since it requires runtime changes?

MulticastEventCallback makes sense. The slight iffy part is that EventCallback wraps MulticastDelegate which kinda has the semantics we want (if you squint at it).

Overall I like this.

SteveSandersonMS commented 2 years ago

Presumably this needs to be behind a LangVersion=7.0 switch since it requires runtime changes?

You're probably right!

The slight iffy part is that EventCallback wraps MulticastDelegate which kinda has the semantics we want (if you squint at it).

Yeah, I did for a while try to work out a way of just using MulticastDelegate to model this, but it doesn't really work because it would be a breaking change for anyone who's already doing interesting things with MulticastDelegate. They already have the behavior that we treat it as returning a single Task, but the new version has to receive a set of Task instances so we can re-render after each handler.

However today I've been reflecting on this further and have started to think this whole "multiple event handlers" concept is solving the wrong problem. I'm now investigating if we can do something very different (and more specific to @bind) which would solve a broader range of problems.

SteveSandersonMS commented 2 years ago

@pranavkm I've written up a very different solution strategy at https://github.com/dotnet/aspnetcore/issues/39837. If we go for that, it would make this "multiple event handlers" concept entirely unnecessary.

SteveSandersonMS commented 2 years ago

Closing in favour of https://github.com/dotnet/aspnetcore/issues/39837