-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix event and JS component args serialization #35038
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
Changes from all commits
19b787b
a53a343
ac1391a
e0cd188
e034491
12f138b
cfc568b
29f5e9b
d5e4bd4
d60eeb6
faf6b50
e08d428
10188aa
fd4ad7a
8692abf
d6614f7
0a52063
1476090
65760af
9bb77ad
2fa4703
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 |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
using System.Text.RegularExpressions; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Components.RenderTree; | ||
using Microsoft.AspNetCore.SignalR.Client; | ||
using Microsoft.AspNetCore.SignalR.Protocol; | ||
using Microsoft.Extensions.DependencyInjection; | ||
|
@@ -160,11 +161,11 @@ public Task ClickAsync(string elementId, bool expectRenderBatch = true) | |
} | ||
if (expectRenderBatch) | ||
{ | ||
return ExpectRenderBatch(() => elementNode.ClickAsync(HubConnection)); | ||
return ExpectRenderBatch(() => elementNode.ClickAsync(this)); | ||
} | ||
else | ||
{ | ||
return elementNode.ClickAsync(HubConnection); | ||
return elementNode.ClickAsync(this); | ||
} | ||
} | ||
|
||
|
@@ -175,7 +176,27 @@ public Task SelectAsync(string elementId, string value) | |
throw new InvalidOperationException($"Could not find element with id {elementId}."); | ||
} | ||
|
||
return ExpectRenderBatch(() => elementNode.SelectAsync(HubConnection, value)); | ||
return ExpectRenderBatch(() => elementNode.SelectAsync(this, value)); | ||
} | ||
|
||
public Task DispatchEventAsync(object descriptor, EventArgs eventArgs) | ||
{ | ||
var attachWebRendererInteropCall = Operations.JSInteropCalls.FirstOrDefault(c => c.Identifier == "Blazor._internal.attachWebRendererInterop"); | ||
if (attachWebRendererInteropCall is null) | ||
{ | ||
throw new InvalidOperationException("The server has not yet attached interop methods, so events cannot be dispatched."); | ||
} | ||
|
||
var args = JsonSerializer.Deserialize<JsonElement>(attachWebRendererInteropCall.ArgsJson); | ||
var dotNetObjectRef = args.EnumerateArray().Skip(1).First(); | ||
var dotNetObjectId = dotNetObjectRef.GetProperty("__dotNetObject").GetInt32(); | ||
|
||
return InvokeDotNetMethod( | ||
null, | ||
null, | ||
"DispatchEventAsync", | ||
dotNetObjectId, | ||
JsonSerializer.Serialize(new object[] { descriptor, eventArgs }, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase })); | ||
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 doing these Ignitor updates to make the Ignitor tests still roughly make sense and not further away from working than before, but they are all quarantined and completely broken even before this PR, as they all time out waiting for renderbatches. Fixing Ignitor completely (or more likely removing it, as we've discussed) is out of scope for this PR. The change you see here retains a meaningful way to dispatch events, since the old way is not longer applicable. |
||
} | ||
|
||
public async Task<CapturedRenderBatch?> ExpectRenderBatch(Func<Task> action, TimeSpan? timeout = null) | ||
|
@@ -483,7 +504,7 @@ private Uri GetHubUrl(Uri uri) | |
} | ||
} | ||
|
||
public async Task InvokeDotNetMethod(object callId, string assemblyName, string methodIdentifier, object dotNetObjectId, string argsJson) | ||
public async Task InvokeDotNetMethod(object? callId, string? assemblyName, string methodIdentifier, object dotNetObjectId, string argsJson) | ||
{ | ||
await ExpectDotNetInterop(() => HubConnection.InvokeAsync( | ||
"BeginInvokeDotNetFromJS", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,11 +105,6 @@ public Task InitializeAsync(ProtectedPrerenderComponentApplicationStore store, C | |
await OnCircuitOpenedAsync(cancellationToken); | ||
await OnConnectionUpAsync(cancellationToken); | ||
|
||
// From this point onwards, JavaScript code can add root components if configured | ||
await Renderer.InitializeJSComponentSupportAsync( | ||
_options.RootComponents.JSComponents, | ||
JSRuntime.ReadJsonSerializerOptions()); | ||
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. This kind of initialization is now done from the |
||
|
||
// Here, we add each root component but don't await the returned tasks so that the | ||
// components can be processed in parallel. | ||
var count = Descriptors.Count; | ||
|
@@ -445,49 +440,6 @@ internal async Task<bool> ReceiveJSDataChunk(long streamId, long chunkId, byte[] | |
} | ||
} | ||
|
||
// DispatchEvent is used in a fire-and-forget context, so it's responsible for its own | ||
// error handling. | ||
public async Task DispatchEvent(JsonElement eventDescriptorJson, JsonElement eventArgsJson) | ||
{ | ||
AssertInitialized(); | ||
AssertNotDisposed(); | ||
|
||
WebEventData webEventData; | ||
try | ||
{ | ||
// JsonSerializerOptions are tightly bound to the JsonContext. Cache it on first use using a copy | ||
// of the serializer settings. | ||
webEventData = WebEventData.Parse(Renderer, JSRuntime.ReadJsonSerializerOptions(), eventDescriptorJson, eventArgsJson); | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Invalid event data is fatal. We expect a well-behaved client to send valid JSON. | ||
Log.DispatchEventFailedToParseEventData(_logger, ex); | ||
await TryNotifyClientErrorAsync(Client, GetClientErrorMessage(ex, "Bad input data.")); | ||
UnhandledException?.Invoke(this, new UnhandledExceptionEventArgs(ex, isTerminating: false)); | ||
return; | ||
} | ||
|
||
try | ||
{ | ||
await Renderer.Dispatcher.InvokeAsync(() => | ||
{ | ||
return Renderer.DispatchEventAsync( | ||
webEventData.EventHandlerId, | ||
webEventData.EventFieldInfo, | ||
webEventData.EventArgs); | ||
}); | ||
} | ||
catch (Exception ex) | ||
{ | ||
// A failure in dispatching an event means that it was an attempt to use an invalid event id. | ||
// A well-behaved client won't do this. | ||
Log.DispatchEventFailedToDispatchEvent(_logger, webEventData.EventHandlerId.ToString(CultureInfo.InvariantCulture), ex); | ||
await TryNotifyClientErrorAsync(Client, GetClientErrorMessage(ex, "Failed to dispatch event.")); | ||
UnhandledException?.Invoke(this, new UnhandledExceptionEventArgs(ex, isTerminating: false)); | ||
} | ||
} | ||
|
||
// OnLocationChangedAsync is used in a fire-and-forget context, so it's responsible for its own | ||
// error handling. | ||
public async Task OnLocationChangedAsync(string uri, bool intercepted) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,22 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.ComponentModel; | ||
using System.Text.Json; | ||
using Microsoft.AspNetCore.Components.Web; | ||
using Microsoft.AspNetCore.Components.Web.Infrastructure; | ||
using Microsoft.JSInterop; | ||
|
||
namespace Microsoft.AspNetCore.Components.Server.Circuits | ||
{ | ||
/// <summary> | ||
/// Intended for framework use only. Not supported for use from application code. | ||
/// </summary> | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public class CircuitJSComponentInterop : JSComponentInterop | ||
internal class CircuitJSComponentInterop : JSComponentInterop | ||
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. This no longer needs to be public because it's no longer called directly through JS interop. It's now called via the new |
||
{ | ||
private readonly CircuitOptions _circuitOptions; | ||
private int _jsRootComponentCount; | ||
|
||
internal CircuitJSComponentInterop( | ||
JSComponentConfigurationStore configuration, | ||
JsonSerializerOptions jsonOptions, | ||
CircuitOptions circuitOptions) | ||
: base(configuration, jsonOptions) | ||
internal CircuitJSComponentInterop(CircuitOptions circuitOptions) | ||
: base(circuitOptions.RootComponents.JSComponents) | ||
{ | ||
_circuitOptions = circuitOptions; | ||
} | ||
|
||
/// <summary> | ||
/// For framework use only. | ||
/// </summary> | ||
[JSInvokable] | ||
public override int AddRootComponent(string identifier, string domElementSelector) | ||
protected override int AddRootComponent(string identifier, string domElementSelector) | ||
{ | ||
if (_jsRootComponentCount >= _circuitOptions.RootComponents.MaxJSRootComponents) | ||
{ | ||
|
@@ -43,11 +28,7 @@ public override int AddRootComponent(string identifier, string domElementSelecto | |
return id; | ||
} | ||
|
||
/// <summary> | ||
/// For framework use only. | ||
/// </summary> | ||
[JSInvokable] | ||
public override void RemoveRootComponent(int componentId) | ||
protected override void RemoveRootComponent(int componentId) | ||
{ | ||
base.RemoveRootComponent(componentId); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -242,21 +242,6 @@ public async ValueTask<bool> ReceiveJSDataChunk(long streamId, long chunkId, byt | |||||||||||||
return await circuitHost.ReceiveJSDataChunk(streamId, chunkId, chunk, error); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public async ValueTask DispatchBrowserEvent(JsonElement eventInfo) | ||||||||||||||
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. Removing this method gives me a pause - I can't find the history for this change (it ends at this PR - #12250), but I vaguely recall we used to plumb events thru JSInterop but special cased it at as a perf optimization. Are we losing out on those benefits as part of this change? 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 only difference we had before by not using JS interop was more scenario-specific logs, which I don't think are very consequential. As for perf, it's not going to be meaningfully different because even before this change, we were JSON-serializing the data before sending it over BlazorPack, which is still equivalent to what JS interop does. But the more fundamental issue is that is has to go via JS interop if we want it to also have JS interop features like transporting 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. Update: I should be clearer about this because there is some extra cost in going through JS interop, but (1) it's the same as we had in 5.0, so not a regression - just not an improvement either, and (2) this sets us up to improve it further in the future if we want. Overview for Blazor Server:
So in effect, this PR removes an optimization we had previously added in a 6.0 preview and takes us back to the same perf characteristics we had in 5.0 and before, in that it goes back to passing the data via a .NET string representation instead of staying as UTF8 all the way through. For WebAssembly, this PR doesn't change the perf characteristics either, because it was already doing event dispatch via JS interop. Of course it would be nice if we didn't have to go through that extra representation, but it's fundamental to how JS interop is defined in terms of exchanging strings (JS strings and .NET strings, not UTF8 bytes). But the good news is that following the layering properly like this PR does means we can improve that layering in the future and would automatically get benefits here. We could eliminate the .NET string representation entirely, as suggested at #35065. |
||||||||||||||
{ | ||||||||||||||
Debug.Assert(eventInfo.GetArrayLength() == 2, "Array length should be 2"); | ||||||||||||||
var eventDescriptor = eventInfo[0]; | ||||||||||||||
var eventArgs = eventInfo[1]; | ||||||||||||||
|
||||||||||||||
var circuitHost = await GetActiveCircuitAsync(); | ||||||||||||||
if (circuitHost == null) | ||||||||||||||
{ | ||||||||||||||
return; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
_ = circuitHost.DispatchEvent(eventDescriptor, eventArgs); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public async ValueTask OnRenderCompleted(long renderId, string errorMessageOrNull) | ||||||||||||||
{ | ||||||||||||||
var circuitHost = await GetActiveCircuitAsync(); | ||||||||||||||
|
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.
This check was unhelpful because the capacity needs to be understood as a max capacity, not a guarantee that it will be filled entirely. I realised that if the JS-side code tries to supply
undefined
, because of JSON serialization conventions, that parameter would be omitted, so it's possible for the number of supplied parameters to be less than the total count.