Restore public API contract on WebAssemblyJSRuntime #19968
Conversation
ghost
added
the
| /// <param name="arg1">The second argument.</param> | ||
| /// <returns>The result of the function invocation.</returns> | ||
| public TResult InvokeUnmarshalled<T0, T1, TResult>(string identifier, T0 arg0, T1 arg1) | ||
| => InvokeUnmarshalled<T0, T1, object, TResult>(identifier, arg0, arg1, null); |
There was a problem hiding this comment.
Was there an important reason to turn all these overloads into extension methods? If there was we can change it back.
There was a problem hiding this comment.
It loosely follows the pattern we have with IJSRuntime - https://github.com/dotnet/aspnetcore/blob/master/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeExtensions.cs#L13 where the most extensive overload was declared in the base class. It would have made sense to keep these as extensions if derived types had to implement every overload
There was a problem hiding this comment.
Makes sense, thanks. Since we’re no longer making these methods overridable, that no longer applies, so I’ll leave them as regular instance methods.
| /// <param name="arg1">The second argument.</param> | ||
| /// <returns>The result of the function invocation.</returns> | ||
| public TResult InvokeUnmarshalled<T0, T1, TResult>(string identifier, T0 arg0, T1 arg1) | ||
| => InvokeUnmarshalled<T0, T1, object, TResult>(identifier, arg0, arg1, null); |
There was a problem hiding this comment.
It loosely follows the pattern we have with IJSRuntime - https://github.com/dotnet/aspnetcore/blob/master/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeExtensions.cs#L13 where the most extensive overload was declared in the base class. It would have made sense to keep these as extensions if derived types had to implement every overload
| { | ||
| switch (identifier) | ||
| { | ||
| case "Blazor._internal.getApplicationEnvironment": |
There was a problem hiding this comment.
Nit: Does it make sense to attach these values to constants? Feel free to consider this out of scope since it looks like the code was using string literals prior.
There was a problem hiding this comment.
If any other cases emerge where we reuse the values, we can make consts then. For as long as there’s only one usage I suspect a literal will be fine.
| jsRuntime.Setup(j => j.InvokeUnmarshalled<string, object, object, byte[]>("Blazor._internal.getConfig", It.IsAny<string>(), null, null)) | ||
| .Returns((byte[])null) | ||
| .Verifiable(); | ||
| public TestWebAssemblyJSRuntimeInvoker(string environment = "Production") |
There was a problem hiding this comment.
Is there any particular reason the default is set to Production in the implementation that is designed to run in tests?
There was a problem hiding this comment.
In the absence of any specific value, the client code assumes Production. This mirrors what ASP.NET Core's hosting does too. Responding with Production would most closely approximate the actual code.
* Restore public API contract on WebAssemblyJSRuntime (#19968) * Shrink icon-512.png (#19999) * Use custom DebugProxyHost to initialize DebugProxy config (#19980) Addresses #19909 * Spruce up WebAssemblyHostEnvironment interface and use (#20008) * Load .dlls/.wasm/.pdb in parallel with dotnet.*.js. Fixes #18898 (#20029) * Add BaseAddress property to WebAssemblyHostEnvironment (#20019) - Adds `BaseAddress` to `IWebAssemblyHostEnvironment` - Uses unmarshalled APIs to extract application host - Move NavigationManager initialization to startup code - Fix subdir mapping in ClientSideHostingTest Addresses #19910 Co-authored-by: Steve Sanderson <[email protected]> Co-authored-by: Safia Abdalla <[email protected]> Co-authored-by: Safia Abdalla <[email protected]>
Follow-up from the sync meeting today. The objective of this PR is to keep the public APIs on
WebAssemblyJSRuntimeto what we want to support longer term.The way I've approached it is not the only way it could be done. Another option would be to add an
internal virtualmethod that all the JS interop methods end up calling, and then use an IVT to override that inside the tests. However that then forces virtual dispatch for every JS interop call in production. In practice that makes no real difference while we're running on the interpreter, but it might be more consequential when we integrate with mixed-mode AoT, because mixed-mode AoT tends to fall back on the interpreter when it sees virtual dispatch.The way I have done it is slightly quirky in that it creates a different internal abstraction that we use in cases where we need unit tests to override behavior, and leaves the hot code paths untouched.
Overall since this is now purely an internal implementation, we're free to switch between different implementations any time we want, so it doesn't matter hugely how exactly it's achieved.