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

New JS interop #27083

Merged
merged 35 commits into from
Oct 21, 2022
Merged

New JS interop #27083

merged 35 commits into from
Oct 21, 2022

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 23, 2022

Addresses #27016
Addresses #26364

Internal Review Topic

Notes

  • This gets the ball rolling with the base cases.
  • Code will live in the topic (code blocks) for release. Later, I'll come back and place the example code into the snippet sample apps at dotnet/blazor-samples.
  • API doc cross-links for the new API won't be available until after release, so I'll circle around later and place links after GA.

❓ on this one: I'd prefer if the examples compose in such a way that both the import example and the export example can live SxS in the same developer test app. I provide cut-'n-paste, fully working examples to readers whenever possible, and they're very popular. I ran into a problem tho ... I can't name the export module Interop and place it into the Interop.cs/Interop class file. It fails. 💥 I have to name it something else ... I use Interop2 on the PR ... to make it work. I don't understand why tho. Anyway, I'm 👂 for your help on that point.

For Dan later ...

  • I'm not aware of the long-term plan for this approach to interop: Will it become a thing for Blazor Server? Is there a plan for this to replace JS interop based on IJSRuntime eventually?
  • I drafted this as a standalone topic (but it can be split easily into the existing topics if you wish🧞‍♂️). Justifications ...
    • This doesn't have a special name. I named it ".NET JavaScript [JSImport]/[JSExport] interop" just to have something to distinguish it, based loosely on your blog post comments. Even with the name that I gave it, it's a bit easier to see that this is a separate interop approach if it's in its own topic.
    • It probably is a bit easier for readers to get the whole explanation (calling both ways) if it's all in one spot ... one topic. We had to split call JS and call .NET into separate topics earlier because our original combined topic became iNsAnElY loooong 😵, but I think we can have a single topic at least for .NET 7. Perhaps at .NET 8 ... especially if this interop approach starts to take over ... this topic goes away and the content is split: The opening bit here goes into the JS Overview topic, the call JS piece is for the Call JS from .NET topic, and the call .NET piece is for the Call .NET from JS topic.

@guardrex guardrex mentioned this pull request Sep 23, 2022
8 tasks
@guardrex guardrex self-assigned this Sep 23, 2022
Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

I like it overall :)

Let's add @maraf as another reviewer. I don't have permissions to do that.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

Updates made, and I'll call this a 'successful draft,' given that apparently I didn't butcher the API 😆.

I'll number these for easy responses ...

Updates

I would not even say and isn't a security concern.

  1. I removed that bit. However, it feels to me like there is some kind of risk to this 😨. Should we ping Barry to ask if he thinks something should be called out?

Consider adding C# namespace to razor file and here as well.

  1. Given that all of my examples across the Blazor docs are meant to be cut-'n-paste, working examples for whatever local test app name that the reader is using, I typically avoid using them. It also usually shortens the code and makes for less boilerplate to consume with the examples. Let's see on the next commit if a few NOTE remarks in the text will work to cover it.

This is interesting comment. The JSObject returned by the ImportAsync is indeed disposable proxy. But even if you dispose it, the JSImport infrastructure will keep the JS side ES6 module alive and callable.

  1. WRT "keep the JS side ES6 module alive and callable" ... but not from that component, which is being disposed at that point, right? I mean should we leave this bit as purely a 'best practices' step in the event that they do maintain such a reference to the object?

Remaining ❓ on module name 💥

  1. I'm still unclear on why I had to name the 2nd example (the call .NET example) module Interop2 in order to make it work SxS with the first example (i.e., in the existing Interop C# class if the reader worked through the first example locally).

    It fails 💥 if I try to just drop the SetWelcomeMessage into the existing Interop C# class with Interop as the module name ...

    [JSImport("setMessage", "Interop")]
    internal static partial void SetWelcomeMessage();
    await JSHost.ImportAsync("Interop", "../Pages/CallDotNet.razor.js");

    I had to give that module a different name there (Interop2) in order to make it work ...

    [JSImport("setMessage", "Interop2")]
    internal static partial void SetWelcomeMessage();
    await JSHost.ImportAsync("Interop2", "../Pages/CallDotNet.razor.js");

    It doesn't seem like there should be a problem using the same module name. Anyway ... what can I say in the text about this? ... and/or how might the code example change to make more sense than just numbering it like that (Interop2), which is a silly hack on my part.

@guardrex guardrex requested a review from maraf September 26, 2022 11:17
@pavelsavara
Copy link
Member

3. WRT "keep the JS side ES6 module alive and callable" ... but not from that component, which is being disposed at that point, right? I mean should we leave this bit as purely a 'best practices' step in the event that they do maintain such a reference to the object?

dotnet GC would collect the proxy anyway at some point. I would not complicate the docs about Dispose() especially on ImportAsync because that's only called once per ES6 module.

Remaining ❓ on module name 💥

4. I'm still unclear on why I had to name the 2nd example (the call .NET example) module `Interop2` 

I just wrote this test, to make sure it's working as expected and it is working for me.

[Fact]
public async Task MultipleImportAsync()
{
    var first = await JSHost.ImportAsync("JavaScriptTestHelper", "./JavaScriptTestHelper.mjs");
    var second = await JSHost.ImportAsync("JavaScriptTestHelper", "./JavaScriptTestHelper.mjs");
    Assert.NotNull(first);
    Assert.NotNull(second);
    Assert.Equal("object", first.GetTypeOfProperty("instance"));
    var instance1 = first.GetPropertyAsJSObject("instance");
    var instance2 = second.GetPropertyAsJSObject("instance");
    Assert.Equal(instance1, instance2);
}

Could you please share more details and also the error you get ?

Also, it should be enough to call ImportAsync once for all the imports from the same ES6 module.

@guardrex
Copy link
Collaborator Author

not complicate the docs about Dispose()

It's against the normal disposal pattern that we pitch. I suspect that this would be more important in a server-side scenario that we aren't covering here. If it's ok with you, let me get Dan's feedback on it just to make sure. I just want to ask because we'd normally guide this way. If we don't show it, we might still leave a note that says something like ... 'client-side code that assigns the JSObject doesn't require disposal of the JSObject because ... blah blah blah' ... whatever it makes sense to say. This would at least harden the doc against devs wondering about it. I feel like I'm going to get asked about it one day if we don't proactively say something.

share more details

The following class results if the reader attempts to work through both examples with the same Interop C# class and naming the module the same way (Interop). It would start with just the GetWelcomeMessage method when they work through the call JS section. Then, they would just naturally think that they can add the SetWelcomeMessage and DotNetInteropCall methods to that class.

I kind'a vaguely feel like I know why this isn't working. The collocated JS files are TWO modules in these examples ... is that right? ... and this C# class is giving both of them the same module name ... and it's then it seems like it's choking on that setup 💥. Anyway ... here's the way I was originally trying to make it work ...

using System.Runtime.InteropServices.JavaScript;
using System.Runtime.Versioning;

[SupportedOSPlatform("browser")]
public partial class Interop
{
    [JSImport("getMessage", "Interop")]
    internal static partial string GetWelcomeMessage();

    [JSImport("setMessage", "Interop")]
    internal static partial void SetWelcomeMessage();

    [JSExport]
    internal static string GetMessageFromDotnet()
    {
        return "¡Hola desde Blazor!";
    }
}

The CallDotNet component is ...

@page "/call-dotnet"
@using System.Runtime.InteropServices.JavaScript

<h1>.NET JS Import/Export Interop (Call .NET)</h1>

<p>
    <span id="result">.NET method not executed</span>
</p>

@code {
    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (OperatingSystem.IsBrowser() && firstRender)
        {
            await JSHost.ImportAsync("Interop", "../Pages/CallDotNet.razor.js");

            Interop.SetWelcomeMessage();
        }
    }
}

... and the collocated JS file contains ...

export async function setMessage() {
  const { getAssemblyExports } = await globalThis.getDotnetRuntime(0);
  var exports = await getAssemblyExports("BlazorSample.dll");

  document.getElementById("result").innerText = exports.Interop.GetMessageFromDotnet();
}

The first example (call JS) from the first section, still works in the app 👍. The 2nd example melts down 💥 with ...

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: Error: Assert failed: setMessage must be a Function but was undefined
      xs@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:52419
      As@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:50492
      _mono_wasm_bind_js_function@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:14:102690
      @:wasm-function[10]:0x0
      @:wasm-function[313]:0x1d4ca
      @:wasm-function[283]:0x1c8cf
      @:wasm-function[221]:0xdfdb
      @:wasm-function[220]:0xce8d
      @:wasm-function[8112]:0x1a1f31
      @:wasm-function[2053]:0x859bd
      @:wasm-function[2058]:0x86025
      @:wasm-function[8522]:0x1bb232
      @:wasm-function[2855]:0xaaf71
      createDotnetRuntime</</Module._mono_background_exec@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:14:133854
      ha@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:84836
      
Error: Assert failed: setMessage must be a Function but was undefined
xs@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:52419
As@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:50492
_mono_wasm_bind_js_function@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:14:102690
@:wasm-function[10]:0x0
@:wasm-function[313]:0x1d4ca
@:wasm-function[283]:0x1c8cf
@:wasm-function[221]:0xdfdb
@:wasm-function[220]:0xce8d
@:wasm-function[8112]:0x1a1f31
@:wasm-function[2053]:0x859bd
@:wasm-function[2058]:0x86025
@:wasm-function[8522]:0x1bb232
@:wasm-function[2855]:0xaaf71
createDotnetRuntime</</Module._mono_background_exec@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:14:133854
ha@https://localhost:7105/_framework/dotnet.7.0.0-rc.1.22426.10.t7u94tgxh6.js:5:84836

We can say in the text the reason that the module is named something else for the 2nd example ... whatever that reason is, but I also think we should use something other than Interop2. That seems arbitrary and kind'a silly to me 😄.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

oh ... and WRT ...

call ImportAsync once for all the imports from the same ES6 module.

Do you mean across components??? That seems kind'a strange because one wouldn't know which component a user would request first. Wouldn't that be fragile? ... or should the module be set up for loading when Blazor itself starts?

I wonder if that aspect is going to need to be explained out further in the text ... i.e. ... they can do it the way that the topic shows it for a one-off module used with one component ... one C# interop class, one dedicated module. If they have a module that's set up to work across components, then it should be loaded at Blazor's start? ... is that right?

🤔 ... I think we'd go with TWO interop C# classes with different module names for the TWO components between these sections ... the TWO collocated JS files. It's ideal if these examples are simple and clear and ✨ Just Work!™ ✨😄 . Then, we can have a new section for a single module that exports JS for multiple components. Perhaps, that section does NOT have a fully-working example but just places some partial code examples to show what to do in that case.

This is a typical doc problem ... several ways to do things but a desire to make the introductory content accessible to devs of all levels and new-to-.NET folks.

@pavelsavara
Copy link
Member

pavelsavara commented Sep 26, 2022

Do you mean across components???

If you have same JS file on the same absolute URL across blazor components, it's still the same ES6 module from JS perspective.
So, if those are separate blazor components, they should have separate JS files.

the URL is relative to dotnet.js not to a blazor component

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

Yes, but is the reason that my first attempt fails because the JS exports fail to work in the 2nd example because I named the module the same thing in the C# class but use two collocated JS files? It seems like that's what the problem is. It's just my lack of understanding of JS modules in the context of the new import/export API that tripped me up.

I think my last idea makes the most sense. Each section here gets it's own "Interop" C# class with different class names ... a collocated module for each component (separate JS files) ... and uses a different module name for each. ....... I think 🤔. That's the base case ... the simplest way to demo these approaches SxS in a single test app.

Then ... I think we'll need a new section on using a single module across components (i.e., NOT using collocated JS). There will be an import/export at Blazor start with ONE module ... one module name ... and hypothetically no wipeout 💥 that way.

I'm just thinking out loud, so please feel free to stop me before I embarrass myself further! 🤣

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

Last commit merely sets up the two sections for separate classes and separate modules. That's at least not going to break when placed into the same test app.

Next, I'll see about working up a section that uses a single module across components. I won't necessarily use fully-working example code tho. I might just call out the deltas from the preceding sections that make it work.

UPDATE: I think I have a version of this now. I'll add a section to cover the scenario to see how it composes. But ...

WRT ...

call ImportAsync once for all the imports from the same ES6 module

I still wouldn't know which component is requested first, so I still call JSHost.ImportAsync in each component.

Let's see how this all composes and then we can call for my immediate 💀 and dismemberment 🔪 when see what I put here 😄. Well, it is that time of year! 🎃

UPDATE: Next commit lays a new section with the single module case, but I did call JSHost.ImportAsync in each component, which might NOT be the right approach, but idk which component is requested first.

@pavelsavara
Copy link
Member

are you also updating the samples repo at the same time ? Is there PR with full code I could look at ?

@guardrex
Copy link
Collaborator Author

are you also updating the samples repo at the same time ?

No, that will come later. For now, it's all just inline code blocks in the topic and a local test app that I'm working with. I normally let things settle on the first release of new bits before moving examples into the snippet sample apps.

I can put my test app up on GH if you like.

@guardrex
Copy link
Collaborator Author

Here it is ... ignore the name of the app, as that goes back to Day 1 when I didn't know exactly what this was ...

https://github.com/guardrex/BlazorWASM70UnmarshalledJS

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

... and it's obvious from that that I don't know how to load the module from one spot at app startup and avoid the calls to JSHost.ImportAsync in each component (CallJavaScript2 and CallDotNet2 components). That seems like a good idea, but we don't cover a Blazor.start or JS initializer way of doing that yet. I think Javier has asked me to work on that in a general way. I just haven't had the time to reach his issue for work ... and I don't know as I sit here how that's composed in code.

@pavelsavara
Copy link
Member

Perhaps you could rename the Interop1.cs to CallDotNet1.razor.cs and put it next to the blazor component.

In my demo I have

public partial class DetectHands // this matches the name of the razor component
{
        public partial class Interop
        {
        }
}

and then in JS I could have
detectHandsExports.DetectHandsJsComponent.DetectHands.Interop.OnResults(component, json);

  • detectHandsExports - assembly level
  • DetectHandsJsComponent - namespace
  • DetectHands blazor component class
  • Interop - interop nested class
  • OnResults exported static method

@guardrex
Copy link
Collaborator Author

Perhaps you could rename the Interop1.cs to CallDotNet1.razor.cs and put it next to the blazor component.

Yes! Good idea. I was trying to use a single class at first, but the new way here for the first two sections will compose even better that way.

@guardrex
Copy link
Collaborator Author

Hold off on the last commit tho. I probably need to refactor that again.

I'm running out of time today to continue. We'll probably need to pick back up on Tuesday morning with the updates. No worries tho AFAIK ... I don't think we're late in any sense.

@guardrex
Copy link
Collaborator Author

mmmmm ... yeah ... I'm running into 😈 namespace collision gremlins 😈 because I can't have the component share a class name with the C# interop class. I didn't name things very well 🙈 on a quick pass ... I don't think I ended up with something following your example.

Yeah ... let me pick back up with this on Tuesday morning. I'll fix it all up along the lines of your last suggestion.

If you know how I load a module to avoid the two JSHost.ImportAsync calls for the new 3rd section, I'll be 👂 for that. Otherwise tho, we can pick back up with that tomorrow, too.

@pavelsavara
Copy link
Member

This is all good, you are learning from trying!
So that you could better anticipate the mistakes users would make and tell them how to avoid it.
Keep trying!

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 26, 2022

Yes! That's at least one reason why they bother even keeping me around here! 🤣

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 20, 2022

@danroth27 ... I think there are just two quick items to resolve and this can be merged ...

Cache Busting

Cache busting guidance wasn't finalized. Do you want me to strike that section?

Naming consistency for the new JS interop

".NET JavaScript [JSImport]/[JSExport] interop (API)" was carried over to the Client-side development topic for naming consistency. ".NET" only meant that this technology is a feature of .NET, not that it can only be used in .NET apps, but I see your point about that construction appearing over in that other new article. I made the change that you suggested on the other PR, but it results in an undesirable inconsistency in naming across the two articles. Should I change the name here in the Blazor article to match?

Everywhere, we'd call this tech (initial mention) ...

JavaScript (JS) [JSImport]/[JSExport] interop API

... and then shortened thereafter to either ...

[JSImport]/[JSExport] interop

... or just ...

JS interop

... if we can get away with it. I was naming it with "[JSImport]/[JSExport]" so as not to confuse anyone about this being different from the current release JS interop.

Copy link
Member

@danroth27 danroth27 left a comment

Choose a reason for hiding this comment

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

A few more minor comments, but otherwise this looks good to me.

@danroth27
Copy link
Member

Cache busting guidance wasn't finalized. Do you want me to strike that section?

@guardrex I didn't see this content in the PR anymore, so I assume you already removed it?

@danroth27
Copy link
Member

Should I change the name here in the Blazor article to match?

@guardrex Can you clarify for me what title change you are proposing?

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 20, 2022

I didn't see this content in the PR anymore, so I assume you already removed it?

I just had a placeholder for it at Line 49, currently commented-out in the text. It sounds like we're not going to guide on that aspect, so I'll remove that.

UPDATE: I just removed it on the last commit.

Can you clarify for me what title change you are proposing?

Yes ... it's just up above ☝️ ... asking if we should remove the ".NET" part everywhere from the naming to make it consistent between the two articles.

☝️ #27083 (comment) ☝️

guardrex and others added 2 commits October 20, 2022 11:34
Co-authored-by: Daniel Roth <daroth@microsoft.com>
@guardrex guardrex closed this Oct 20, 2022
@guardrex guardrex reopened this Oct 20, 2022
@danroth27
Copy link
Member

asking if we should remove the ".NET" part everywhere from the naming to make it consistent between the two articles.

The current title for this article is: ".NET JavaScript [JSImport]/[JSExport] interop with ASP.NET Core Blazor"
And title of the other article is: "Run .NET from JavaScript using [JSImport]/[JSExport] interop"

Are you proposing that the name of this article should drop the ".NET" for consistency? So the new title would be: "JavaScript [JSImport]/[JSExport] interop with ASP.NET Core Blazor"? If so, I'm fine with that.

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 21, 2022

Yes, that's right. I've made the change. It's consistent with the other article now.

This is the first Blazor doc to get file-scoped namespaces. Updates will roll into all of the docs over time. I noticed that MS/you aren't fully collapsing the namespace line under the usings (or making it the first line of the file) to save one more vertical line. I figured it's worth asking about on ...

Why not save one more line with file-scoped namespaces? (dotnet/aspnetcore #44676)
dotnet/aspnetcore#44676

For this PR, I do it the PU-way ... e.g. ...

using System.Runtime.InteropServices.JavaScript;
using System.Runtime.Versioning;

namespace BlazorSample.JavaScriptInterop;

[SupportedOSPlatform("browser")]
public partial class Interop
{
    ...
}

If you change your mind, I'll patch these up later.

🤔

Personally ... just for my own personal use ... I kind'a favor at the moment going top-of-file with it ...

namespace BlazorSample.JavaScriptInterop;
using System.Runtime.InteropServices.JavaScript;
using System.Runtime.Versioning;

[SupportedOSPlatform("browser")]
public partial class Interop
{
    ...
}

That locates it quickly, which is all that placing it on a line with space around it seems to accomplish.

@guardrex guardrex merged commit 401d45e into main Oct 21, 2022
@guardrex guardrex deleted the guardrex/blazor-new-js-interop branch October 21, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants