JavaScript interop v3 #1033
JavaScript interop v3 #1033
Conversation
| DotNet.attachDispatcher({ | ||
| beginInvokeDotNetFromJS: (callId, assemblyName, methodIdentifier, argsJson) => { | ||
| monoPlatform.callMethod(dotNetDispatcherBeginInvokeMethodHandle, null, [ | ||
| callId ? monoPlatform.toDotNetString(callId.toString()) : null, // TODO: Implement a way to pass a number to a long without stringification |
There was a problem hiding this comment.
Am still planning to do this "TODO" item.
There was a problem hiding this comment.
Changed my mind. The two approaches I considered both don't fit well.
- Allocating the 8 bytes on the stack and passing directly as a raw value parameter: The existing code that prepares the params array on the stack assumes 4 bytes per param, and generalising it will come at a perf cost for every invocation, plus complicate the
Platforminterface considerably. - Passing as a boxed long: Mono doesn't currently expose any convenient APIs for allocating a heap object of an arbitrary type. And even if it did that's still a heap allocation, so the perf gain over passing as a string is not massive.
For now I'm keeping the "pass as string" behavior. It's not optimal for perf but it only affects this one code path. We can reconsider in the future if we decide it's consequential.
| return RegisteredFunction.Invoke<string>( | ||
| ShowPromptIdentifier, | ||
| return JSRuntime.Current.InvokeAsync<string>( | ||
| "TestContentPackage.showPrompt", // Keep in sync with identifiers in the.js file |
There was a problem hiding this comment.
After we merge this PR to dev and there is a new build on the dev feed, we need to remember to go back to the BlazorLib project template and make the equivalent change to this. Can't do so until there is a new build on the dev feed with the new APIs in it.
There was a problem hiding this comment.
Reminder - we need to do this 😆
There was a problem hiding this comment.
Yep, done in 36dc336
Awkwardly the new convention of making the JS functions globally-reachable via an expression of the form window.something.somethingElse.etcetera means there's no longer a simple way to auto-generate different per-project namespaces matching the project name in JS code. This is because AFAICT dotnet new doesn't have a built-in way of saying you want to inject the "safe" version of the project name in specific places, so we can't use the project name in the JS code because it might contain dots, and our lookup logic interprets dots as property name separators.
Net result is that I've now hard-coded the namespace of the showPrompt example function as exampleJsFunctions, which I think is adequate to make the point that developers should change it to something that makes sense in their project.
The only way this would be a problem is if people actually start shipping NuGet packages in which the JS functions are still attached to a global called exampleJsFunctions, and then trying to use multiple such packages would lead to name clashes. But I think that's extremely unlikely and would obviously be a package authoring error.
There was a problem hiding this comment.
Yeah, I would not bother with it... A comment in the docs saying that people should change to a meaningful (and safe) name is necessary for sanity...
| @@ -4,13 +4,14 @@ | |||
| "description": "", | |||
| "main": "index.js", | |||
| "scripts": { | |||
| "build": "webpack", | |||
| "build": "webpack --mode development", | |||
There was a problem hiding this comment.
This is just retaining the existing behavior. We're tracking "do minification" in #1003.
| renderBatch, | ||
| http: httpInternalFunctions, | ||
| uriHelper: uriHelperInternalFunctions | ||
| } |
There was a problem hiding this comment.
I can see that you're doing a relayer on some of the internals of blazor.js - I'm mostly ignoring these since I'm not sure I would have any meaningful feedback.
There was a problem hiding this comment.
So as I understand it - you're making everything was a registered function into a function that's reachable on some path from the root (window) since that's the new mechanism we're using to bind functions.
|
|
||
| namespace Microsoft.AspNetCore.Blazor.Browser.Interop | ||
| { | ||
| internal class ArgumentList |
There was a problem hiding this comment.
Yep, the updated equivalent to this is: https://github.com/aspnet/Blazor/pull/1033/files#diff-b80ce524aaadc1a948d2f5b50db36160R77
Instead of having to create a special wire format for args arrays and then a lot of .NET-side logic for parsing and extracting values, it was possible just to JSON.serialize the whole args array on the JS side with no preprocessing, and then do a two-phase deserialize on the .NET side. It shouldn't cost anything extra in perf as in effect it was doing this before but just not retaining the information it needed.
| { | ||
| // When implementing support for an out-of-process JS runtime, we'll need to | ||
| // do something here to serialize and transmit the RenderBatch efficiently. | ||
| throw new NotImplementedException("TODO: Support BrowserRenderer.UpdateDisplay on other runtimes."); |
There was a problem hiding this comment.
Wouldn't that be another class though (not BrowserRenderer)? I'd imagine this could just do a hard cast like L68
We'd start by writing another class, and then would never arrive at this TODO since we'd be writing another class 😆
There was a problem hiding this comment.
Yes, that's possible! I strongly expect we'll be tweaking the layering a bit when/if we implement another Renderer, since there is probably some aspects of BrowserRenderer that we'd want to share across renderers. The exact layer boundaries probably have to move around a bit.
| // Temporarily enable MonoWebAssemblyJSRuntime on this class constructor | ||
| // Later it will become part of the app startup and config mechanism | ||
| JSRuntime.SetCurrentJSRuntime(new MonoWebAssemblyJSRuntime()); | ||
| } |
There was a problem hiding this comment.
I have some thoughts about how this fits in to the overall picture - this hack seems fine for now.
There was a problem hiding this comment.
Great, looking forwards to tying all this together.
| static BrowserServiceProvider() | ||
| { | ||
| // TODO: Remove once we make this part of the app startup mechanism | ||
| ActivateMonoJSRuntime.EnsureActivated(); |
There was a problem hiding this comment.
You might want to return something and do a GC.KeepAlive() on the result. I'm not sure if the linker could remove this or not.
There was a problem hiding this comment.
Good point. Done.
The linker wasn't stripping it out before, but maybe an updated linker might be smart enough to do so. So I agree it's a good idea to GC.KeepAlive on a returned object to ensure it isn't stripped.
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\Microsoft.JSInterop\Microsoft.JSInterop.csproj" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
It looks like the only reason for this reference is that ElementRef does custom serialization. Is that the case? Is this worth thinking harder about? (probably not) I can't imagine anyone will build a Blazor application without the interop layer.
There was a problem hiding this comment.
It's also to make the JSON utils available, as they are used in a few places in this project.
It might be that one day we can reference some other JSON utils from here and not reference Microsoft.JSInterop. For the time being I expect it's fine as-is.
| * Invokes the specified .NET public method synchronously. Not all hosting scenarios support | ||
| * synchronous invocation, so if possible use invokeMethodAsync instead. | ||
| * | ||
| * @param assemblyName The name of the .NET assembly containing the method. |
There was a problem hiding this comment.
Do you think it's worth specifying in the docs that its the short assembly name (without key and version)? If you're coming from a .NET background you might assume that it's the full name
There was a problem hiding this comment.
Good point. Done.
| } | ||
|
|
||
| function parseJsonWithRevivers(json: string): any { | ||
| return json ? JSON.parse(json, (key, initialValue) => { |
There was a problem hiding this comment.
Interesting... I wasn't aware this feature exists 👍
There was a problem hiding this comment.
Yeah, typescript language features are amazing... Each version it surprises me more! 👍
There was a problem hiding this comment.
Well I this case I think it's browser APIs - but yes, Typescript is amazing. 😻
| void *jsCallResult = (void *)EM_ASM_INT ({ | ||
| char *argsJsonUtf8 = argsJson == NULL ? NULL : mono_string_to_utf8 (argsJson); | ||
|
|
||
| MonoString *resultJsonMonoString = (void *)EM_ASM_INT ({ |
There was a problem hiding this comment.
So I am assuming that this block is 'inline assembly' in the form of JS.
S P I C Y
There was a problem hiding this comment.
| Timeout.Infinite); | ||
|
|
||
| return tcs.Task; | ||
| await Task.Delay(300); |
There was a problem hiding this comment.
You can also use Task.Yield() here if you want to avoid a slowing down the test significantly. That's the documented way to actually force asynchrony
There was a problem hiding this comment.
Good tip - done.
|
Ok. So you really inflated the number of lines changed by moving mono.asm.js. T hese are the kinds of shenanigans that put @pranavkm in first place in the MVC repo https://github.com/aspnet/Mvc/graphs/contributors - I don't approve #salty |
| DotNet.attachReviver((key, value) => { | ||
| if (value && typeof value === 'object' && value.hasOwnProperty(elementRefKey) && typeof value[elementRefKey] === 'number') { | ||
| return getElementByCaptureId(value[elementRefKey]); |
There was a problem hiding this comment.
Should this be looking at the key? here? I suppose it won't be a problem because the top-value object is always an array?
There was a problem hiding this comment.
If I understand your question correctly then no, it shouldn't be looking at key. If we receive the following:
{"someElem":{"_blazorElementRef":123}}
Then we'd receive a call to the reviver with key="someElem" and value={_blazorElementRef:123}. The key doesn't matter; we just need to check that the value looks like an ElementRef, and return an HTMLElement instance obtained by reading the _blazorElementRef property from value. Then the resulting deserialized object will look like:
{ someElem: AnHTMLElementInstance }
There was a problem hiding this comment.
Ah ok, you are correct. Thanks for clarifying
| /// <param name="methodIdentifier">The identifier of the method to be invoked. The method must be annotated with a <see cref="JSInvokableAttribute"/> matching this identifier string.</param> | ||
| /// <param name="argsJson">A JSON representation of the parameters.</param> | ||
| /// <returns>A JSON representation of the return value, or null.</returns> | ||
| public static string Invoke(string assemblyName, string methodIdentifier, string argsJson) |
There was a problem hiding this comment.
It might be worth a comment explaining why this and BeginInvoke don't need [JSInvokable]
There was a problem hiding this comment.
Good point. Added the following comment to both:
// This method doesn't need [JSInvokable] because the platform is responsible for having
// some way to dispatch calls here. The logic inside here is the thing that checks whether
// the targeted method has [JSInvokable]. It is not itself subject to that restriction,
// because there would be nobody to police that. This method *is* the police.
| // Invoke and coerce the result to a Task so the caller can use the same async API | ||
| // for both synchronous and asynchronous methods | ||
| var task = syncResult is Task syncResultTask ? syncResultTask : Task.FromResult(syncResult); | ||
| task.ContinueWith(completedTask => |
There was a problem hiding this comment.
Do you anticipate any problems if the continuation executes synchronously? I believe the existing callers of this method currently behave correctly
There was a problem hiding this comment.
I don't think there will be any problem with that, because this method is void, so the caller can't be relying on receiving any synchronous output before the continuation is invoked.
Also since the other side of the interop boundary is getting the continuation as a Promise resolution, that already guarantees as per the Promise spec that resolutions will be delivered asynchronously. So in practice I don't think it will be possible to observe the callback arriving synchronously.
If anyone ever manages to create a scenario where the difference is observable, we can tweak this to force an async continuation.
| catch (Exception ex) | ||
| { | ||
| ex = UnwrapException(ex); | ||
| jsRuntimeBaseInstance.EndInvokeDotNet(callId, false, ex.ToString()); |
There was a problem hiding this comment.
Is there any use case here for passing the exception object? It seems little odd to me that this layer would decide how you want to treat exceptions.
There was a problem hiding this comment.
That's a good point. Agreed and updated.
| .Where(method => method.IsDefined(typeof(JSInvokableAttribute), inherit: false)) | ||
| .ToDictionary( | ||
| method => method.GetCustomAttribute<JSInvokableAttribute>(false).Identifier, | ||
| method => (method, method.GetParameters().Select(p => p.ParameterType).ToArray()) |
There was a problem hiding this comment.
There's a few other things you can do here that wouldn't be supported - like generics and parameter modifiers (out/ref). Is it worth trying to throw if you put JSInvokable on an invalid method?
There was a problem hiding this comment.
I'm not too worried about that, as it will already throw with a clear message when you try to actually invoke it (e.g., "not a closed generic type"). If we try to anticipate all the cases here there might be a lot to consider.
| /// <param name="identifier">An identifier for the function to invoke. For example, the value <code>"someScope.someFunction"</code> will invoke the function <code>window.someScope.someFunction</code>.</param> | ||
| /// <param name="args">JSON-serializable arguments.</param> | ||
| /// <returns>An instance of <typeparamref name="T"/> obtained by JSON-deserializing the return value.</returns> | ||
| T Invoke<T>(string identifier, params object[] args); |
There was a problem hiding this comment.
Would about void-returning methods? Is the expectation that you'd use object as the type?
There was a problem hiding this comment.
Yes, we have been using <object> for that (the compiler doesn't allow void or System.Void). I'm aware it's a bit clunky and we can add another overload for that if people turn out to want it.
| /// <returns>An instance of <typeparamref name="T"/> obtained by JSON-deserializing the return value.</returns> | ||
| public Task<T> InvokeAsync<T>(string identifier, params object[] args) | ||
| { | ||
| // TODO: Auto-timeout |
There was a problem hiding this comment.
I'd suggest against this if you don't have a clear need in mind.
There are lots of cases where you don't want to have a timeout - "I want to be notified if X happens, but it might never happen".
If you want a timeout, then you want it to be cooperative - in which case you'd want to send it to the JS side as an actual argument - but I don't believe promises have a concept like cancellation - so again, you're building your own protocol.
If you want a timeout but you're not going to make it cooperative (client cancellation) then what you'd want to cancel is any continuations you have scheduled, which is pretty easy to do in .NET.
There was a problem hiding this comment.
OK, I'll leave it for now, but will have a comment there that it might need to be reviewed.
My concern is that in the server-executed case, it's preferable not to be leaking memory if the JS-side code is just badly written and doesn't notify about completion (e.g., on errors, or if it's a void function and the developer hasn't thought about what they are doing). Plus, an individual user could deliberately change the JS-side code running in their browser so it doesn't notify completion even if the developer wrote their app correctly.
We might decide that a timeout is not the best solution even still. Maybe there should be a default per-JS-runtime limit on the number of pending tasks. This would limit it separately for each connected user.
| /// <param name="argsJson">A JSON representation of the arguments.</param> | ||
| protected abstract void BeginInvokeJS(long asyncHandle, string identifier, string argsJson); | ||
|
|
||
| internal void EndInvokeDotNet(string callId, bool success, object resultOrExceptionMessage) |
There was a problem hiding this comment.
With these begin-end pattern things, it's possible to have a bug where begin/end are called on different runtime instances - (if anything changes the instance of the runtime during execution). I don't think it's likely that anyone will do that - but just in case.
There was a problem hiding this comment.
It is possible, that's true. But our work on sync context should make it easier to get this right than wrong in typical cases. In the event that a developer somehow still gets it wrong, it should be pretty detectable because there will be a lot of errors of the form "there is no pending task with ID x". If you have an idea for how we should improve this please let me know :)
| // This is a single-file self-contained module to avoid the need for a Webpack build | ||
|
|
||
| module DotNet { | ||
| (window as any).DotNet = DotNet; // Ensure reachable from anywhere |
There was a problem hiding this comment.
Any reason why this isn't camelCase? just struck me as odd
There was a problem hiding this comment.
I'm thinking of DotNet as being like a static class. It's conventional for class names to be PascalCase in JS, hence static-method-like APIs such as Promise.race, WebAssembly.compile, etc.
| { | ||
| if (!_pendingTasks.TryRemove(asyncHandle, out var tcs)) | ||
| { | ||
| throw new ArgumentException($"There is no pending task with handle '{asyncHandle}'."); |
There was a problem hiding this comment.
I imagine that this exception would only been seen by something like our interop transports - or in the case of mono it can bubble back to JS. We'll have to make sure to handle arbitrary exceptions in those cases unless we want the app to crash.
There was a problem hiding this comment.
It should be hard to trigger this, but if you somehow do, it implies something is pretty badly wrong.
In client-side execution, a global unhandled exception is a reasonable outcome. For server-side execution, the code that invokes DotNetDispatcher.Invoke/BeginInvoke should try/catch it and do something sensible with unhandled exceptions.
There was a problem hiding this comment.
should try/catch it and do something sensible with unhandled exceptions.
I'm interested if you have more thoughts, but the only sensible thing I can think of would be to log, or optionally destroy the circuit (seems bad). We don't have the context of a user-operation when this happens because it happens in on resuming async.
|
So I'm imagining that for server and B-lectron we'll have a 'channel' that does remote IO and is responsible for calling the same methods that are called by monoplatform.js. That's the location where we'll do things like call in the appropriate sync context and ensure that the runtime is initialized in the call-context. It seems simple enough. Let me know if you had a different idea. |
| /// <summary> | ||
| /// Represents an instance of a JavaScript runtime to which calls may be dispatched. | ||
| /// </summary> | ||
| public interface IJSRuntime |
There was a problem hiding this comment.
Should we expose any sort of features/capabilities here? Did you plan to add that through more interfaces?
There was a problem hiding this comment.
Yes, the interfaces mechanism was working fine to distinguish cases where sync or unmarshalled interop is supported. Maybe that will be enough. If not we can figure out what extra capability advertisement would be appropriate when we have scenarios for it.
Yes, exactly! And on the .NET side it will call the same methods on |
* JavaScript interop via new IJSRuntime abstraction * CR feedback
This is one of the big pieces needed for #959. It changes how the JS interop works so it's factored out of the Blazor codebase itself, and makes "async" calls the mainstream lowest-common-denominator case (with sync and unmarshalled calls still available via certain casts if you really need that).
It's a monstrously big PR, but it does all have to happen in one go because once you change the underpinnings there are so many consequences. @rynowak, we can discuss whether or how you want to review this.
I'll write up a list of changes (especially the breaking API changes) in here later. For now I just want the CI run to start so am submitting this.