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

Design proposal: Multiple same-named event handlers #39815

Closed
SteveSandersonMS opened this issue Jan 27, 2022 · 5 comments
Closed

Design proposal: Multiple same-named event handlers #39815

SteveSandersonMS opened this issue Jan 27, 2022 · 5 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 27, 2022

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 #14365 and is well upvoted. It deals with the following pain points:

  • Being able to use @bind and @onchange (or other bind-supported events) at the same time, letting developers perform additional actions before or after binding
  • More generally reaching parity with HTML elements and addEventListener.

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

  • Back-compat in all cases with Razor syntax. All existing valid RZC-emitted code should not be able to observe any change of behavior. This includes when @attributes overrides or is overridden by another event handler.
  • Developer controls the order of handler execution. We'll use the left-to-right source code order. The diffing algorithm mustn't lose this order information.
  • Each handler should behave independently of others. For example:
    • ComponentBase components should re-render after each async handler's returned Task, not waiting for them all to complete. However, there should not be a synchronous re-render after invoking each handler, as it sufficies to do a single synchronous render after they have all started.
    • Each handler may have a different target, and hence any subset of those targets may transition their enclosing ErrorBoundary into an error state independently of the others, and that doesn't stop subsequent handlers from running.
  • Compiler/tooling error updates
    • Given <button @onclick=A @onclick=B>, the existing tooling shows a squiggly-line warning for the second (the code compiles, but produces invalid logic that throws at runtime). Tooling needs to stop showing a warning.
    • Given <input @bind=A @onchange=B>, the existing compiler emits diagnostic RZ10008 and fails compilation. We will remove this diagnostic completely.
  • Having as many handlers as you want. Even though there are very few use cases, it will be easiest to understand if there's no upper bound on how many @onchange handlers can go on a single element.
  • Pay only if you use it. There should be no perf degradation (e.g., extra allocations, or any new multi-step process during diffing) for the single-handler case. However it's OK for this new functionality to incur some modest extra cost since it will be rarely used.

Out of scope

  • Back-compat in edge cases with manual RenderTreeBuilder logic.

    • Example: currently, you can call AddAttribute twice with the same event handler name. The existing behavior is to only run the second handler. This is arbitrary and unsupported, so it's OK for the behavior here to change.
    • As it happens, my proposed design would retain back-compat even here, but that's just lucky and not a goal
  • Components receiving multiple values for arbitrary parameters. So, when used on components (not elements) this only affects the CaptureUnmatchedValues behavior. Examples:

    • Will work: <MyComponent @onclick=A @onclick=B />
    • Will work: <MyInput @bind-Value=A @onchange=B>
    • Will work: <MyInput @bind-Value=A OnChange=B>
    • Won't work: <MyComponent OnClick=A OnClick=B>.

    This is because, for arbitrary parameters like OnClick, there's no built-in concept that they represent events. There's no strong use case for OnClick=A OnClick=B, as the person doing this can always just make a single method that calls A and B.

  • Razor syntax for customizing whether @attributes overrides or appends to the set of handlers for a given event name.

    • Example: <button @onclick=A @attributes=B>. If B contains an onclick, this should only call that one, and not A. This is needed for back-compat.
    • Example: <button @attributes=A @onclick=B>. This should only call B, regardless of what's in A. This is needed for back-compat.

    Developers who want arbitrary rules for appending or replacing event handlers should be able to do so by cracking open the CaptureUnmatchedValues dictionary by hand, but we won't extend the build-in @attributes syntax for this.

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.

  • Pro: feels like this is how you'd design it from scratch.
  • Con: every aspect of the system, such as diffing, event tracking, and event dispatch, all would have to change.

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.

  • Pro: some existing diffing and event tracking/dispatch logic would just work because the names are distinct
  • Con: diffing's attribute overriding logic would get very confused, and we might incur extra runtime costs to preserve ordering when doing the dictionary-based attribute diffs
  • Con: customer code that cares about attribute names would encounter new cases it perhaps doesn't handle

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.

  • Pro: all existing diffing logic would just work
  • Pro: naturally retains existing overriding rules for back-compat
  • Con: how does it represent multiple handlers?

This leads to sub-cases:

  • C1. Razor compiler codegens a single method that calls A then B, and uses that as the delegate.
    • Pro: no new runtime logic needed - it's all in the compiler
    • Con: extremely hard or impossible to generate code that works as desired. Since the generated delegate can only return a single Task, how can we re-render after each inner handler? How do we make it preserve ErrorBoundary semantics if the handlers have different targets?
  • C2. Define some new event callback type that represents a collection of event callbacks. The renderer can know about this, and call each of its inner handlers in order, receiving a separate Task from each.
    • Pro: relatively simple change to Razor compiler
    • Con: another concept to wield inside the renderer

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:

  • Option A doesn't work, because the dictionary can't have multiple entries with the same key
  • Option B is really bad, because the mangled names become visible to user code, which can get confused about them or take a dependency on the exact form of mangling
  • Option C is fine with this

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:

  • EventCallbackGroup
  • EventCallbackCollection
  • MulticastEventCallback

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:

  • Then we need to update some aspects of event handler assignment and event dispatch logic to know about MulticastEventCallback-typed attribute values.
  • In fact, the renderer's underlying storage of Dictionary<ulong, EventCallback> _eventHandlers will have to change to Dictionary<ulong, MulticastEventCallback>. This should not cause any extra allocations for existing single-handler cases.
@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description labels Jan 27, 2022
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 27, 2022
@mkArtakMSFT mkArtakMSFT added this to the 7.0-preview2 milestone Jan 27, 2022
@mkArtakMSFT
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

@pranavkm I've written up a very different solution strategy at #39837. If we go for that, it would make this "multiple event handlers" concept entirely unnecessary.

@SteveSandersonMS
Copy link
Member Author

Closing in favour of #39837

@ghost ghost locked as resolved and limited conversation to collaborators Mar 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

5 participants