How to retain EventHandlerId

[uazo]

Consider this:

<button class="btn btn-primary"
        onclick="@(e => IncrementCount(1))">
    Click me +1
</button>

@for (int t = 2; t != 4; t++)
{
    <button class="btn btn-primary"
            onclick="@(e => IncrementCount(t))">
        Click me +@t
    </button>
}

@functions {
  int currentCount = 0;

  void IncrementCount(int t)
  {
      currentCount += t;
      this.StateHasChanged();
  }

blazor will generate three EventHandlerId.
When I press the button, the first EventHandlerId is retained (just because the ladba "point" at the same address) but the others are simple new labda expression, so blazor will delete and recreate the handler. In a list with many elements and many handler, it's a performance problem.

a way to retain can be:

@for (int t = 2; t != 4; t++)
{
    <button class="btn btn-primary"
            onclick="@Action(t, e => IncrementCount(t))">
        Click me +@t
    </button>
}


  Dictionary<MethodInfo, Dictionary<object, Action<UIEventArgs>>>
    _retainedActions = new Dictionary<MethodInfo, Dictionary<object, Action<UIEventArgs>>>();

  public Action<UIEventArgs> Action<T>(T args1, Action<UIEventArgs> action)
  {
      if (_retainedActions.TryGetValue(action.Method, out var list) == false)
      {
          list = new System.Collections.Generic.Dictionary<object, Action<UIEventArgs>>();
          _retainedActions[action.Method] = list;
      }

      if (list.TryGetValue(args1, out var value) == false)
      {
          value = (e) => action(e);
          list[args1] = value;
      }
      return value;
  }

other ideas?

[uazo]

I've found another way: blazor can only change the delegate for eventBindings dictionary, keeping same EventHandlerId.
I do not think there are contraindications: @SteveSandersonMS what do you think?

here https://github.com/uazo/Blazor/tree/retain-eventhandlerid code and samples

...
                if (oldFrame.AttributeEventHandlerId > 0)
                {
                    // DONT REMOVE, SIMPLY CHANGE
                    //diffContext.BatchBuilder.DisposedEventHandlerIds.Append(oldFrame.AttributeEventHandlerId);
                    diffContext.Renderer.AssignEventHandlerId(ref newFrame, oldFrame.AttributeEventHandlerId);
                    newFrame = oldFrame;
                }
...

    internal void AssignEventHandlerId(ref RenderTreeFrame frame, int id)
    {
        if( id <= 0 ) id = ++_lastEventHandlerId;

        if (frame.AttributeValue is MulticastDelegate @delegate)
        {
            if (_eventBindings.ContainsKey(id))
                _eventBindings[id] = new EventHandlerInvoker(@delegate);
            else
                _eventBindings.Add(id, new EventHandlerInvoker(@delegate));
        }

        frame = frame.WithAttributeEventHandlerId(id);
    }

[SteveSandersonMS]

@uazo I agree it would be great if we could do that. But won't this break cases where the event handler is meant to change?

Example:

<button onclick=@clickHandler>Click me</button>

@functions {
    Action clickHandler = () => {
        Console.WriteLine("First click");

        // Now change the delegate
        clickHandler = () => {
            Console.WriteLine("Second click"); 
        };
    };
}

[uazo]

that code doesn't compile...

Making it static all is working as expected.
It's because EventHandlerId is a marker for the javascript code for the current delegate (c# side), regardless of what it is.

Maybe a problem happens for nested events, with multiple StateHasChanged(), just because is StateHasChanged (when calling ProcessRenderQueue) to assign the current EventHandlerInvoker. So, if a code change the delegate in a event, and in the browser there is another event that what to call the older delegate, my code doesn't works.

I try to code an example.

[uazo]

there is no way (for now :) to generate nested events.
I tried with webcomponents but also the current code returns unexpected results

[uazo]

Trying with https://github.com/aspnet/Blazor/issues/802#issuecomment-387953426, it works very well.

[arivera12]

I am stuck with this problem as well. Its kind of funny how this is working. Consider the next code and how it behaves where i is an index:

onclick="@OnClick(i)"

In this approach I get the correct index of the record but all records OnClick event gets fired.

onclick="@(() => OnClick(i))"

In this approach only one event gets fired but the index is wrong, it turns to be the length of the array.

I have spend days trying to do a work around it and I just went out of ideas for it...

[Andrzej-W]

@arivera12

In this approach only one event gets fired but the index is wrong, it turns to be the length of the array.

I don't see your code but maybe you have problem like this: https://github.com/aspnet/Blazor/issues/1402

[arivera12]

@Andrzej-W

I don't see your code but maybe you have problem like this: aspnet/Blazor#1402

This worked as expected and yes that was my issue! Thanks so much!

[dmorrison42]

Beyond the performance concerns mentioned above this (anti?-) pattern seems to trigger #6385.

I have a table (with lambda event handlers) that grows rapidly to a couple hundred elements, and ends up failing to AssignEventHandlerId within less than a minute of use.

I was able to work around it by creating a new element and passing the value as a parameter removing the need for a lambda.

Before:

@for (var i = 0; i < Messages.Count; i++) {
    var message = Messages[i];
    <tr ondblclick="@(() => MessageDoubleClick(message))"> 
    ...

After:

@for (var i = 0; i < Messages.Count; i++) {
            <MessageRow Message="@Messages[i]" OnDoubleClick="@MessageDoubleClick"></MessageRow>
}

It's a little verbose, but hopefully it can get anyone hitting this issue moving until the issue is addressed.

[mkArtakMSFT]

@SteveSandersonMS is #8232 the desired resolution of this?

[SteveSandersonMS]

No, I'm not aware that there is any resolution for this. In general we'd have to make the compiler able to know whether a given delegate can change or not, and if it can't, implement some caching logic. It's outside the scope of anything we plan to do in the short term.

One useful benefit of moving to newer versions of C# is that the compiler does get smarter about it. For event handlers that are methodgroups (e.g., onclick=@SomeMethod), the compiler is increasingly able to inject its own methodgroup-to-delegate-conversion cache field on the class. Last time I checked it only worked with static methods, but maybe it will get support for instance methods later.

Since I don't think there's any action for us to take, I'm closing this.

CC @rynowak in case he has any more knowledge about where the C# compiler is going with this, or in case he has any new ideas about what we might do inside the Razor compiler to improve it. Ryan, if you do think we could do anything here (even in the long term), please feel free to reopen.

[uazo]

you must excuse me for the late reply.

I agree it would be great if we could do that. But won't this break cases where the event handler is meant to change?

Maybe I don't understand but we don't have to check if the delegate is the same or not, my code always replaces it!
I don't modify the EventHandlerId to avoid triggering the javascript "handler elimination/generation" part, and in server-side it reduces the frame size.
That is "Same EventHandlerId but different delegate"

Invocation:

        public virtual Task DispatchEventAsync(int eventHandlerId, UIEventArgs eventArgs)
        {
            EnsureSynchronizationContext();

The callback is referenced by eventHandlerId   vvvvv
            if (!_eventBindings.TryGetValue(eventHandlerId, out var callback))
            {
                throw new ArgumentException($"There is no event handler with ID {eventHandlerId}");
            }

            Task task = null;
            try
            {
                // The event handler might request multiple renders in sequence. Capture them
                // all in a single batch.
                _isBatchInProgress = true;

                task = callback.InvokeAsync(eventArgs);
            }

Assignment:

    internal void AssignEventHandlerId(ref RenderTreeFrame frame, int id)
    {
        if( id <= 0 ) id = ++_lastEventHandlerId;

        if (frame.AttributeValue is MulticastDelegate @delegate)
        {
            if (_eventBindings.ContainsKey(id))
                _eventBindings[id] = new EventHandlerInvoker(@delegate);
            else
                _eventBindings.Add(id, new EventHandlerInvoker(@delegate));
        }

        frame = frame.WithAttributeEventHandlerId(id);
    }


Note: the code must be updated with the current master, but the logic is always the same.

Why we have to Delete and Recreate the id, after all in the events if something changes is only c# side, in javascript the reference is always to the same event, "onclick" for example (and necessarily because AppendDiffEntriesForAttributeFrame takes for granted that oldTree and newTree refer to the same attribute of the same element).

        // This should only be called for attributes that have the same name. This is an
        // invariant maintained by the callers.
        private static void AppendDiffEntriesForAttributeFrame(

what am I missing here?

[SteveSandersonMS]

It's not safe to replace change which delegate an event handler ID refers to. If we did that, then it's possible that:

• A user clicks "delete" on the first item in a list
• Before the message reaches the server, some unrelated render changes the order of the list, so a different thing is now at the top
• The "delete" message reaches the server, and an unexpected item gets deleted

Keeping the event-handler-ID-to-delegate mapping preserves the association between a user action and whatever data is captured within the delegate's lambda.