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

Custom event args #29993

Merged
merged 25 commits into from
Feb 18, 2021
Merged

Custom event args #29993

merged 25 commits into from
Feb 18, 2021

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Feb 8, 2021

Implements #17552

The overall design matches what is described in the proposal at #17552, so please see that for an overview of the changes in this PR. I'll add comments on individual lines for more clarity.

Some high-level implementation notes:

  • While I was in the process of changing a lot of event dispatch stuff, I took the opportunity to tidy it up quite a lot. Previously the BrowserRenderer and EventDispatcher were excessively interlinked, with the event dispatch code paths repeatedly bouncing back and forth between the two. I've teased these two apart so now there's almost nothing left to do with event dispatch inside BrowserRenderer, and all the event-related code is away in a separate subdirectory that mostly just works in isolation.
  • The protocol for communicating events from JS to .NET is now simpler:
    • Previously, the JS side code contained logic to map the raw DOM event types to one of several "categories" (e.g, copy, cut, and paste were all mapped to the clipboard category). JS would send this category name to .NET, and .NET would use to to determine which EventArgs subclass to use for deserializing the args data.
    • This notion of predefined categories no longer makes sense now the event types can be entirely custom. So now, the JS side code sends the original event name to .NET (with no category), and the .NET-side code chooses how to deserialize it. For the built-in web event names, we have built-in logic mapping them to EventArgs subclasses (e.g., paste maps to ClipboardEventArgs). For custom event names, the .NET code looks at the event's registered delegate and gets its parameter type, validating that there's only one and it does inherit from EventArgs. Altogether this is simpler from the JS side, which allows for some of the decoupling I did.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Feb 8, 2021
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/custom-event-args-real branch from 6b739ae to 763db33 Compare February 10, 2021 11:45
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review February 10, 2021 15:57
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner February 10, 2021 15:57
}
else
{
throw new InvalidOperationException($"The event handler parameter type {declaredType.FullName} for event must inherit from {typeof(EventArgs).FullName}.");
Copy link
Member Author

Choose a reason for hiding this comment

The 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 EventArgs is something we've made assumptions about in other places, plus it's good to make the developer really indicate for sure that they want to deserialize arbitrary untrusted input into this type.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 [EventHandler(...)]). Having two indicators (subclassing EventArgs and explicitly putting it on one of these definitions) is a clear enough statement of intent IMO.

The risk is similar to a [FromBody] SomeType myParam on an MVC action method. In that case we consider it a sufficiently clear statement of intent with fewer steps and restrictions.

{
// For custom events, the args type is determined from the associated delegate
eventArgsType = renderer.GetEventArgsType(eventHandlerId);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic above is where we've changed from the old "category"-based type selection to the newer rules, i.e., for built-in known event names we have hardcoded logic, and for unknown event names we get the type from the delegate.

The reason we have to have the hardcoded logic for known event names (and can't just use the delegate in all cases) is for back-compat. People might set up an event handler delegate that receives EventArgs and expect that, at runtime, they receive whichever subclass corresponds to the actual event. We retain that edge-case feature for built-in event names, but for custom events you only receive what you declare.

Also it's quite nice that the built-in events don't have to even look at the delegate and remain closer to their historical implementation. Less chance of any obscure behavioral changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expectation that you author a custom JsonConverter if you needed to handle complex deserialization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, same as with a JS interop call.

@@ -8,7 +8,7 @@ import { RenderQueue } from './Platform/Circuits/RenderQueue';
import { ConsoleLogger } from './Platform/Logging/Loggers';
import { LogLevel, Logger } from './Platform/Logging/Logger';
import { CircuitDescriptor } from './Platform/Circuits/CircuitManager';
import { setEventDispatcher } from './Rendering/RendererEventDispatcher';
import { setEventDispatcher } from './Rendering/Events/EventDispatcher';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll see quite a bit of renaming and file moving in this PR for the TypeScript "events" related code. This is to clean up all kinds of cruft that had accumulated in this area.


// Make the following APIs available in global scope for invocation from JS
window['Blazor'] = {
navigateTo,
registerCustomEventType,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API REVIEW This is new public API on the JavaScript side (Blazor.registerCustomEventType). See the implementation at https://github.com/dotnet/aspnetcore/pull/29993/files#diff-58576c4c052d6fbfb851ec2f4b3bf740a2e318f0a9aeec9bc7aaf7a8acffe2eaR12 for details of the args.

I deliberately chose a hyperspecific name for this because (1) it's a pretty uncommon and advanced thing, and (2) I want to clarify that this is for registering the event type, and not something else like "event args mapping rule" which might be something else we add as a different feature in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you been thinking about how we would provide a typing (completion) experience as we continue to add newer JS APIs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We have talked about shipping a .d.ts file for the public APIs, but I wonder if some smaller change to the structure of blazor.*.js would be enough to make VS able to infer the exposed APIs and offer them based on the presence of the <script src="blazor.*.js"> alone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can put a comment reference to the d.ts in index.html somehow to have editor's pick it up. we could put a reference to the tagged version for each release on our repo.

@@ -1,15 +1,11 @@
import { RenderBatch, ArrayBuilderSegment, RenderTreeEdit, RenderTreeFrame, EditType, FrameType, ArrayValues } from './RenderBatch/RenderBatch';
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various event-related things were decoupled from BrowserRenderer here. Mostly the spaghettification was not for any good reason and just evolved there as we added more and more event-related features over time.

@@ -1,378 +0,0 @@
export class EventForDotNet<TData extends UIEventArgs> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is retired in favour of the newer EventTypes.ts

@@ -1,18 +1,24 @@
import { EventDescriptor } from './BrowserRenderer';
import { UIEventArgs } from './EventForDotNet';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I eliminated the UIEventArgs type because it didn't serve any purpose. From the JS side, event args objects are now just any (makes sense because they can be supplied by arbitrary developer-provided callbacks).

createEventArgs: e => parseWheelEvent(e as WheelEvent)
});

registerBuiltInEventType(['toggle'], createBlankEventArgsOptions);
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the logic for built-in event types here is just a reformulated version of what we used to have in EventForDotNet.ts before. The built-in event types are held in the same data structure we use for custom event registrations, so it's easy to treat them all equivalently when processing events, and we can guarantee uniqueness of names.

Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string!
Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.set -> void
*REMOVED*Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventArgsType.get -> string!
*REMOVED*Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventArgsType.set -> void
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although these two removals look like public API changes, they are inside the RenderTree namespace that explicitly warns people not to use it. There has always been an analyzer that tells people that any uses of these types are subject to breaking changes.

Also, the WebEventDescriptor.EventArgsType should never have been used in any application code, since it's the protocol for dispatching events from JS to .NET, and user application code is not involved in that process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the WebEventDescriptor.EventArgsType should never have been used in any application code, since it's the protocol for dispatching events from JS to .NET, and user application code is not involved in that process.

Could you file an announcement for this? We didn't explicitly warn users against using it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do - #30275

@@ -19,6 +19,7 @@ public static class WebEventCallbackFactoryEventArgsExtensions
/// <param name="receiver">The event receiver.</param>
/// <param name="callback">The event callback.</param>
/// <returns>The <see cref="EventCallback"/>.</returns>
[Obsolete("This extension method is obsolete and will be removed in a future version. Use the generic overload instead.")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these obsoletions are for #28414

@@ -26,7 +26,6 @@ public class EventTest : ServerTestBase<ToggleExecutionModeServerFixture<Program
protected override void InitializeAsyncCore()
{
Navigate(ServerPathBase, noReload: true);
Browser.MountTestComponent<EventBubblingComponent>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This served no purpose

<div __internal_stopPropagation_onclick="@intermediateStopPropagation">
<button id="button-with-onclick" @onclick="@(() => LogEvent("target onclick"))" __internal_stopPropagation_onclick="@targetStopPropagation">
<div @onclick:stopPropagation="@intermediateStopPropagation">
<button id="button-with-onclick" @onclick="@(() => LogEvent("target onclick"))" @onclick:stopPropagation="@targetStopPropagation">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are not strictly required for the PR, but I'm tidying up as I go

<p>
Later, it's likely that we'll add a syntax for controlling whether any given event handler
triggers a synchronous <code>preventDefault</code> before the event handler runs.
</p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did that. Comment is redundant.

@@ -27,7 +27,7 @@
<option value="BasicTestApp.ErrorComponent">Error throwing</option>
<option value="BasicTestApp.EventBubblingComponent">Event bubbling</option>
<option value="BasicTestApp.EventCallbackTest.EventCallbackCases">EventCallback</option>
<option value="BasicTestApp.EventCasesComponent">Event cases</option>
<option value="BasicTestApp.EventCustomArgsComponent">Event custom arguments</option>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not renaming or replacing here, even though it looks like it in the diff.

I'm removing EventCasesComponent because it doesn't exist and hasn't done for a long time - this is an orphaned link. And separately, I'm adding EventCustomArgsComponent.

@@ -39,56 +44,68 @@
{
DumpEvent(e);
message += "onmouseover,";
StateHasChanged();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these StateHasChanged calls should have been there. They're just not needed.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/custom-event-args-real branch from 276d73f to 0686d1a Compare February 10, 2021 18:31
{
internal static class EventArgsTypeCache
{
private static ConcurrentDictionary<MethodInfo, Type> Cache = new ConcurrentDictionary<MethodInfo, Type>();
Copy link
Member

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.

Copy link
Member Author

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

/// </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)
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is preventing us from building a map at host initialization time of (eventName -> type) and using that for the deserialization?

That information doesn't exist in any formal way. The [EventHandler] registrations are a convention for the Razor compiler, but developers aren't compelled to use it.

The information that does exist for sure is the mapping from eventHandlerId->Delegate, which the renderer already stores.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That information doesn't exist in any formal way. The [EventHandler] registrations are a convention for the Razor compiler, but developers aren't compelled to use it.

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.

// some listeners for it are already present. Once the event name alias gets registered,
// we have to notify any existing event delegators so they can update their delegated
// events list.
eventNameAliasRegisteredCallbacks.forEach(callback => callback(eventName, options.browserEventName));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example on when you think this is useful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. If we didn't do this, we'd be relying on the event handlers being registered on the JS side before the corresponding elements get added to the DOM. This would be really inconvenient because you'd now be forced to use the autostart=false thing on <script src="blazor.*.js"> so you could be sure to register the event handler before the first render. It would be worse still for component library authors, as they'd have to do some kind of deferred async rendering flow to guarantee their event handler registrations go through before their component(s) first appear in the UI.

I judged that these usability issues were unacceptable and needed a solution. So with the mechanism you see here, the event handler can be registered after the elements are rendered in the DOM. The only requirement is that the event handler is registered before the event actually occurs (i.e., in response to a user action), of course.

@@ -71,7 +71,7 @@ public async Task DispatchingAnInvalidEvent_DoesNotTriggerWarnings()
{
BrowserRendererId = 0,
EventHandlerId = 1990,
EventArgsType = "mouse",
EventName = "click",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the reason for the property name change, but why are we changing the value here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because mouse was the name of the category (which included click events), and we're no longer supplying category names. We're now supplying the actual event name, which in this case is click.

@onkeydown="@(e => { LogMessage("Received native keydown event"); })"
@ontestevent="@HandleTestEvent"
@onkeydown.testvariant="@HandleCustomKeyDown"
@onkeydown.yetanother="@HandleYetAnotherKeyboardEvent"
Copy link
Member

@javiercn javiercn Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the invocation order for callbacks when you have multiple registered event handlers like this for the same event?

I'm assuming keydown will trigger both events.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically like native DOM events, in that they occur in some particular order in practice, but you shouldn't rely on it: https://stackoverflow.com/questions/282245/dom-event-precedence. For native DOM events, it may vary across browsers.

In our implementation it would fire the native event first, and then the aliases would fire in order based on registration. But since browsers themselves don't define this officially, I hope we don't need to either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

I was asking since we use lexical order for things like @splat attributes and wanted to make sure that if there was such thing in the DOM we considered respecting it.

[EventHandler("onkeydown.testvariant", typeof(TestKeyDownEventArgs), true, true)]
[EventHandler("onkeydown.yetanother", typeof(YetAnotherCustomKeyboardEventArgs), true, true)]
[EventHandler("oncustommouseover", typeof(EventArgs), true, true)]
public static class EventHandlers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still struggling a bit with this.

It feels more natural for EventHandler to be applied directly to the custom event class. That way everything is in one place and I think it's a nice gesture to force using a custom type if you want to define a special event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok if we think this is out of the scope of this PR, but I truly think we should change the way this works to avoid having to use this class moving forward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.

I agree we could make this nicer. But [EventHandler] is what we've already got and need to remain back-compatible with. This PR doesn't change the .NET-side registration mechanism. If we want to create a different and nicer registration mechanism, it can be looked at separately if customer demand justifies it. This PR doesn't lock us any more closely to the existing registration style.

@SteveSandersonMS
Copy link
Member Author

@dotnet/aspnet-blazor-eng Ping for review so we can get this into preview 2

{
// For custom events, the args type is determined from the associated delegate
eventArgsType = renderer.GetEventArgsType(eventHandlerId);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the expectation that you author a custom JsonConverter if you needed to handle complex deserialization?


// Make the following APIs available in global scope for invocation from JS
window['Blazor'] = {
navigateTo,
registerCustomEventType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you been thinking about how we would provide a typing (completion) experience as we continue to add newer JS APIs?

Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string!
Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.set -> void
*REMOVED*Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventArgsType.get -> string!
*REMOVED*Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventArgsType.set -> void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the WebEventDescriptor.EventArgsType should never have been used in any application code, since it's the protocol for dispatching events from JS to .NET, and user application code is not involved in that process.

Could you file an announcement for this? We didn't explicitly warn users against using it.


namespace Microsoft.AspNetCore.Components.E2ETest.Tests
{
public class EventCustomArgsTest : ServerTestBase<ToggleExecutionModeServerFixture<Program>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for validating you can't replace an existing event?

@SteveSandersonMS SteveSandersonMS merged commit d97be90 into main Feb 18, 2021
@SteveSandersonMS SteveSandersonMS deleted the stevesa/custom-event-args-real branch February 18, 2021 22:46
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

I have a few more comments, but I think we can get this merged and resolve them in the future.

My biggest concern is the deserialization of "Random" custom types, but I don't think I have a good solution for it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants