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

Run .NET from JS article #27311

Merged
merged 12 commits into from
Oct 26, 2022
Merged

Run .NET from JS article #27311

merged 12 commits into from
Oct 26, 2022

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Oct 18, 2022

@guardrex guardrex self-assigned this Oct 18, 2022
@guardrex guardrex mentioned this pull request Oct 18, 2022
@guardrex guardrex mentioned this pull request Oct 18, 2022
8 tasks
@pavelsavara
Copy link
Member

cc @radical

@guardrex
Copy link
Collaborator Author

I took into account all of the feedback. Check the language translations ... .NET guru to HackRex 🦖 ... for errors. I may have inadvertently broken a few concepts in the process. 🙈

The table will get WCAG updates later ... immediately prior to merging this. For now, we can look at the simple layout on the PR to troubleshoot it further. If I add the WCAG bits now, it will be much harder to read on the diff and update.

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.

Looking really good! Just a few more changes.

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 21, 2022

Updates made.

Since the module filename doesn't seem to be an important concept here, I 🔪 that out of the coverage. Now, this focuses on the module name (main.js), not the filename. That seems like it will ward off some confusion there. See if my changes are 👍 or 😵.

NOTE TO SELF: 🎗️ Don't forget to WCAG the table before merging! 🎗️

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 22, 2022

Updates made.

  • I changed MemoryView to IMemoryView in the table and down the bulleted list.
  • I updated the definition of IMemoryView and removed the cross-link to marshal.ts.
  • I commented-out the .NET ref source remark just in case we need it later. If we end up not needing it, I'll 🔪 it.

NOTE TO SELF 🦖: 🎗️ Don't forget to WCAG the table before merging! 🎗️

@FranklinWhale
Copy link

FranklinWhale commented Oct 22, 2022

Updates made.

  • I changed MemoryView to IMemoryView in the table and down the bulleted list.
  • I updated the definition of IMemoryView and removed the cross-link to marshal.ts.
  • I commented-out the .NET ref source remark just in case we need it later. If we end up not needing it, I'll 🔪 it.

NOTE TO SELF 🦖: 🎗️ Don't forget to WCAG the table before merging! 🎗️

  • IMemoryView
    • IMemoryView is a JS type of .NET interop, not a primitive JS type.
    • Bytes aren't copied during marshalling.
    • IMemoryView created for a Span is only valid for the duration of the interop call.
    • IMemoryView created for an ArraySegment survives after the interop call and is useful for sharing a buffer.
    • IMemoryView doesn't have an analogous JS type, so marshalling a JS object to a .NET IMemoryView isn't possible.
  • It's not possible to export a .NET method that returns a Span. The Span is allocated on the call stack and has GC implications. When calling from JS to .NET, there's no C# stack after the call.
  • For an exported method that returns an ArraySegment, calling dispose() in try-finally block disposes the proxy and unpins the underlying C# byte array. We recommend calling dispose() on the object in developer JS code. If developer code doesn't dispose of the object, the JS GC eventually disposes the object. You can also marshal a byte array (byte[]) instead of an ArraySegment, which copies the bytes.

@pavelsavara @guardrex MemoryView implements IMemoryView and IDisposable, and that's why MemoryView has dispose() and isDisposed. By specifying IMemoryView only, I think developers may lose sight of IDisposable.

I also think that some information in the quoted text above may be further clarified. May I propose changing to the following?

  • Array
    • Marshalling an array creates a copy of the array in JavaScript or .NET, as the case may be.
  • MemoryView
    • MemoryView is a JavaScript class for the .NET WebAssembly runtime to marshal Span and ArraySegment.
    • Unlike marshalling an array, marshalling a Span or ArraySegment does not create a copy of the underlying memory.
    • MemoryView can only be properly instantiated by the .NET WebAssembly runtime. Therefore, it is not possble to import a JavaScript function as a .NET method that has a parameter of Span or ArraySegment.
    • MemoryView created for a Span is only valid for the duration of the interop call. As Span is allocated on the call stack, which does not persist after the interop call, it is not possible to export a .NET method that returns a Span.
    • MemoryView created for an ArraySegment survives after the interop call and is useful for sharing a buffer. Calling dispose() on a MemoryView created for an ArraySegment disposes the proxy and unpins the underlying .NET array. We recommend calling dispose() in try-finally block for all such MemoryView.

I have add a point to explictly state that marshalling an array creates a copy of the array, and therefore the code below will not work as intended:

[JSExport]
// This method will not actually modify the array in JS, as a copy of it is passed to .NET during marshalling
static void ModifyArrayFromJS(string[] message) {
	message[0] = "Override";
	Console.WriteLine(String.Join(',', message));
}

I have deleted If developer code doesn't dispose of the object, the JS GC eventually disposes the object. because it is not clear when that will happen eventually.

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 25, 2022

@danroth27 ... WRT naming/titling: In this part of the ToC (client-side development), we don't need to distinguish this interop from an existing JS interop (i.e., IJSRuntime JS interop), so it isn't necessary to rely so much on the 'import-export' phrasing for naming/titling. Also, '.NET interop' seems like a possible replacement for 'JS interop' in this context (but only in titling/naming) because the primary usefulness is for JS apps/JS devs to call .NET ... based on how you titled it (i.e., 'run .NET from JS'). It's possible that the naming/titling should reflect that focus.

I made a change on the last commit to see how things might compose. What do you think about ...

  • Title: Run .NET from JavaScript
  • Filename: client-side/dotnet-interop.md
  • URL: https://learn.microsoft.com/aspnet/core/client-side/dotnet-interop
  • UID: client-side/dotnet-interop
  • ToC entry name: Run .NET from JavaScript

... is that too distinct from 'JS interop', such that it would cause confusion? If so, we could just drop the 'import-export' language and go with something like ...

  • Title: Run .NET from JavaScript
  • Filename: client-side/javascript-interop.md
  • URL: https://learn.microsoft.com/aspnet/core/client-side/javascript-interop
  • UID: client-side/javascript-interop
  • ToC entry name: Run .NET from JavaScript

Either way, I still propose that we maintain the formal technology name in the articles' guidance as JavaScript (JS) [JSImport]/[JSExport] interop in order to have one name for it across client-side and Blazor articles. It's easy to update the content later to just "JS interop" if this API replaces IJSRuntime interop. The filename/UID are more painful to change, so I hope we set up this client-side article in a way that won't require filename/UID changes later.

NOTE TO SELF: 🎗️ Don't forget to WCAG the table before merging! 🎗️

@danroth27
Copy link
Member

I made a change on the last commit to see how things might compose

Looks good to me.

I still propose that we maintain the formal technology name in the articles' guidance as JavaScript (JS) [JSImport]/[JSExport] interop in order to have one name for it across client-side and Blazor articles

That's fine. It's a bit wordy, but I think it's workable for now. I'll let you know if I come up with something cleaner.

It's easy to update the content later to just "JS interop" if this API replaces IJSRuntime interop

We still expect to steer Blazor users toIJSRuntime so that their JS interop logic works across all Blazor hosting models.

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.

:shipit:

@guardrex
Copy link
Collaborator Author

guardrex commented Oct 25, 2022

🎉

One more quick pass on Wednesday morning, and I'll merge this and take it live.

NOTE TO SELF: 🎗️ Don't forget to WCAG the table before merging! 🎗️ Done! 👍

@guardrex guardrex merged commit cb049c2 into main Oct 26, 2022
@guardrex guardrex deleted the guardrex/client-new-js-interop branch October 26, 2022 10:47
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.

New Blazor JS interop
5 participants