-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Custom event args #29993
Custom event args #29993
Changes from all commits
8e5e7d4
ea7c7c2
7b58208
b37c11a
eb391df
60d67eb
f0a78bb
6d2694d
1792351
badc38f
c832baa
08967e8
66f7dba
c9ca4b8
11cbd20
6ca5e3a
c1523ff
3a9c621
34c4338
c09a2f1
1ebdf84
907999d
9ad4039
8c41111
5c49dac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Reflection; | ||
|
||
namespace Microsoft.AspNetCore.Components.RenderTree | ||
{ | ||
internal static class EventArgsTypeCache | ||
{ | ||
private static ConcurrentDictionary<MethodInfo, Type> Cache = new ConcurrentDictionary<MethodInfo, Type>(); | ||
|
||
public static Type GetEventArgsType(MethodInfo methodInfo) | ||
{ | ||
return Cache.GetOrAdd(methodInfo, methodInfo => | ||
{ | ||
var parameterInfos = methodInfo.GetParameters(); | ||
if (parameterInfos.Length == 0) | ||
{ | ||
return typeof(EventArgs); | ||
} | ||
else if (parameterInfos.Length > 1) | ||
{ | ||
throw new InvalidOperationException($"The method {methodInfo} cannot be used as an event handler because it declares more than one parameter."); | ||
} | ||
else | ||
{ | ||
var declaredType = parameterInfos[0].ParameterType; | ||
if (typeof(EventArgs).IsAssignableFrom(declaredType)) | ||
{ | ||
return declaredType; | ||
} | ||
else | ||
{ | ||
throw new InvalidOperationException($"The event handler parameter type {declaredType.FullName} for event must inherit from {typeof(EventArgs).FullName}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't really have to have this rule, but I'd err on the side of strictness. We can always loosen such rules later, but can't tighten them. Inheriting from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's legit to force inheriting from EventArgs. As you mention, we need a gesture to mark these types as suitable for custom events. That said, I'm not 100% sure extending EventArgs is enough for this. Mainly because many types outside Blazor extend from it and we would want to avoid those "spurious" usages. Maybe we can mark it with an attribute instead to be more explicit. Specially since this has security implications in SSB There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other gesture a developer does to opt into this is actually using the type as the parameter to an event handler definition (with The risk is similar to a |
||
} | ||
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -249,11 +249,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie | |
{ | ||
Dispatcher.AssertAccess(); | ||
|
||
if (!_eventBindings.TryGetValue(eventHandlerId, out var callback)) | ||
{ | ||
throw new ArgumentException($"There is no event handler associated with this event. EventId: '{eventHandlerId}'.", nameof(eventHandlerId)); | ||
} | ||
|
||
var callback = GetRequiredEventCallback(eventHandlerId); | ||
Log.HandlingEvent(_logger, eventHandlerId, eventArgs); | ||
|
||
if (fieldInfo != null) | ||
|
@@ -291,6 +287,24 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie | |
return result; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the event arguments type for the specified event handler. | ||
/// </summary> | ||
/// <param name="eventHandlerId">The <see cref="RenderTreeFrame.AttributeEventHandlerId"/> value from the original event attribute.</param> | ||
/// <returns>The parameter type expected by the event handler. Normally this is a subclass of <see cref="EventArgs"/>.</returns> | ||
public Type GetEventArgsType(ulong eventHandlerId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the reason for this being public that we need to call it from other assemblies? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and because it's really becoming a part of the contract a renderer makes with its host. The web host has reasons for wanting to know the .NET type to supply for an event argument, so other hosts may want to do that too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unclear why we need to do this based on the event handler id vs based on the event name since there can't be two registrations for the same name. What is preventing us from building a map at host initialization time of (eventName -> type) and using that for the deserialization? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That information doesn't exist in any formal way. The The information that does exist for sure is the mapping from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Then the comment here is not completely correct, nothing prevents me from using a random type as an event handler provided that it inherits from EventArgs. |
||
{ | ||
var methodInfo = GetRequiredEventCallback(eventHandlerId).Delegate?.Method; | ||
|
||
// The DispatchEventAsync code paths allow for the case where Delegate or its method | ||
// is null, and in this case the event receiver just receives null. This won't happen | ||
// under normal circumstances, but to avoid creating a new failure scenario, allow for | ||
// that edge case here too. | ||
return methodInfo == null | ||
? typeof(EventArgs) | ||
: EventArgsTypeCache.GetEventArgsType(methodInfo); | ||
} | ||
|
||
internal void InstantiateChildComponentOnFrame(ref RenderTreeFrame frame, int parentComponentId) | ||
{ | ||
if (frame.FrameTypeField != RenderTreeFrameType.Component) | ||
|
@@ -404,6 +418,16 @@ internal void TrackReplacedEventHandlerId(ulong oldEventHandlerId, ulong newEven | |
_eventHandlerIdReplacements.Add(oldEventHandlerId, newEventHandlerId); | ||
} | ||
|
||
private EventCallback GetRequiredEventCallback(ulong eventHandlerId) | ||
{ | ||
if (!_eventBindings.TryGetValue(eventHandlerId, out var callback)) | ||
{ | ||
throw new ArgumentException($"There is no event handler associated with this event. EventId: '{eventHandlerId}'.", nameof(eventHandlerId)); | ||
} | ||
|
||
return callback; | ||
} | ||
|
||
private ulong FindLatestEventHandlerIdInChain(ulong eventHandlerId) | ||
{ | ||
while (_eventHandlerIdReplacements.TryGetValue(eventHandlerId, out var replacementEventHandlerId)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a concurrent dictionary here? Or are all uses within the sync context? The reason that I'm asking is because this hurts "linkability" in the wasm cases, and I believe there are a bunch of cases where we are using it and we don't need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's shared across all circuits