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

Enhanced JSImport/JSExport guidance #33156

Merged
merged 24 commits into from
Aug 22, 2024

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jul 24, 2024

Fixes #32001

Fantastic article @SerratedSharp! 🎷

Notes:

  • Code lines were adjusted to meet our 85 character line length limit.
  • In a few spots, I changed "function" to "method" because it looked like the text was referring to calling the JS declaration on a JS object. As you review, file a suggestion for any line that should say "method" instead of "function." Check the remaining mentions of "function" to confirm that they're correct.
  • In one of the code comments, there's an error message shown: "ToJS for System.Int64 is not implemented." What is "ToJS" there? 👂
  • The original client-side .NET interop article will need to stay on the PR because GH 💥😈 for some cryptic reason when I tried to merely move and rename the file. I'll investigate and deal with it separately before going live.
  • I'm changing the prereqs for the Blazor article to VS, instead of saying the prereq is just the .NET SDK ... and I added that they should install the .NET WebAssembly build tools during a VS install. Let me know if I'm incorrect on that aspect. 👂

Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/javascript-interoperability/call-javascript-from-dotnet.md Call JavaScript functions from .NET methods in ASP.NET Core Blazor
aspnetcore/blazor/javascript-interoperability/import-export-interop.md JavaScript JSImport/JSExport interop with ASP.NET Core Blazor
aspnetcore/client-side/dotnet-interop.md Run .NET from JavaScript
aspnetcore/client-side/dotnet-interop/index.md aspnetcore/client-side/dotnet-interop/index
aspnetcore/client-side/dotnet-interop/wasm-browser-app.md aspnetcore/client-side/dotnet-interop/wasm-browser-app
aspnetcore/release-notes/aspnetcore-7.0.md aspnetcore/release-notes/aspnetcore-7.0
aspnetcore/whats-new/dotnet-AspNetCore.Docs-mod0.md ASP.NET Core docs: What's new for June 2024

@guardrex guardrex changed the title Guardrex/blazor import export interop updates Enhanced JSImport/JSExport guidance Jul 24, 2024
@guardrex guardrex self-assigned this Jul 24, 2024
@guardrex guardrex marked this pull request as ready for review July 25, 2024 13:16
@SerratedSharp

This comment was marked as resolved.

@guardrex
Copy link
Collaborator Author

guardrex commented Jul 26, 2024

Thanks ... I'll make updates. For the future, note that we use GH's suggestion feature for line changes, which is efficient for change requests on PRs.

UPDATE: Done! ... You can let me know if further changes are needed. Otherwise, I think Pavel and/or Mackinnon will take a look at the technical aspects. I think Dan will let us know if the overall content layout makes sense.

Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

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

Just adding some thoughts - I think we should wait for a review from @pavelsavara before merging.

aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
Co-authored-by: Mackinnon Buck <mackinnon.buck@gmail.com>
Co-authored-by: Mackinnon Buck <mackinnon.buck@gmail.com>
@guardrex
Copy link
Collaborator Author

guardrex commented Jul 29, 2024

I still need an answer on if the .NET WebAssembly build tools option must be included for Visual Studio use (the Blazor ariticle). The client-side article states (emphasis added) ...

Install the wasm-tools workload in an administrative command shell, which brings in the related MSBuild targets:

dotnet workload install wasm-tools

The tools can also be installed via Visual Studio's installer under the ASP.NET and web development workload in the Visual Studio installer. Select the .NET WebAssembly build tools option from the list of optional components.

Brings in the related MSBuild targets implies that devs need the tools for the Blazor article experience with VS.

If the client-side article is incorrect, then let me know 👂, and I'll 🔪 that out of both articles.

@ilonatommy
Copy link
Member

I still need an answer on if the .NET WebAssembly build tools option must be included for Visual Studio use (the Blazor ariticle). The client-side article states (emphasis added) ...

Empirically tested, it's not needed., I removed it from the machine. To see the browser WASM template in VS we can use:

dotnet new install Microsoft.NET.Runtime.WebAssembly.Templates

and having no wasm-tools nor wasm-experimental, I am able to run the template that contains JSImport/Export operations.

However, you are asking about "Blazor experience" while the quoted step is about "WebAssembly Browser App", so please, review if I did not misunderstand your question, I am testing WebAssembly Browser App.

aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
aspnetcore/client-side/dotnet-interop/index.md Outdated Show resolved Hide resolved
Co-authored-by: SerratedSharp <97156524+SerratedSharp@users.noreply.github.com>
@guardrex
Copy link
Collaborator Author

guardrex commented Aug 7, 2024

Thanks @SerratedSharp ... Sorry that I didn't see your remark from three days ago. Thanks for providing the updates. I merged all of them except for the one that you're discussing with @pavelsavara on adding back "ES6," which I'll commit if he's cool with it. Done! 👍

Next, I'll pick back up with testing within the next couple of days, hopefully tomorrow morning 🤞.

guardrex and others added 4 commits August 7, 2024 10:40
Co-authored-by: SerratedSharp <97156524+SerratedSharp@users.noreply.github.com>
@guardrex
Copy link
Collaborator Author

guardrex commented Aug 8, 2024

@SerratedSharp ... That licked it 🎉 ...

dotnet/blazor-samples#324

Anything else for this?

@pavelsavara
Copy link
Member

@pavelsavara
Copy link
Member

pavelsavara commented Aug 8, 2024

Unrelated to this PR, I'm thinking that Blazor users should be nudged to use workload and compatible flags/features which improve performances/size. Are the samples the right place ?

Should we demo Blazor compatible things like <InvariantTimezone>true</InvariantTimezone> and <InvariantGlobalization>true</InvariantGlobalization> there ?

Should there be AOT demo ?

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 8, 2024

<InvariantGlobalization> is covered in the glob/loc article.

<InvariantTimezone> can be covered near it, so I've opened #33292 to address it.

Not necessarily an AOT "demo" (i.e., sample app), but we do have AOT coverage.

Generally, when you write "demo" IDK if you mean "sample app" or just "coverage." I'm trying to limit the number of sample apps. They're time-consuming to maintain, and there are a bunch of them now ...

https://github.com/dotnet/blazor-samples/tree/main/8.0

They all must be updated every release.

@pavelsavara
Copy link
Member

I was thinking sample, maybe one of the existing samples could have it all enabled ? No pressure, just idea.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 8, 2024

I'm not feel'in it because ...

  • It requires the tooling install, and I'd like the existing samples to ✨Just Work!™✨ OOB for the scenarios that they cover.
  • After the tooling is installed, the changes to an app to adopt AOT are trivial and don't demonstrate anything if done to an existing app or new app unless presented SxS with code to enable/disable AOT (or yet another sample) that would run slowly without AOT.
  • AFAIK, DR doesn't want anything to adopt AOT OOB. That's my vague recollection from discussing how AOT would be covered when it was first introduced at 6.0. You can ping him, of course, to see if he has a change of ❤️ about it.

@pavelsavara
Copy link
Member

It requires the tooling install, and I'd like the existing samples to ✨_**Just Work!**_™✨ OOB for the scenarios that they cover.

That's DX of a lazy developer ;)
If you care about UX of your app users, you could reconsider.

enable/disable AOT (or yet another sample) that would run slowly without AOT.

AOT is faster in execution, but much slower for dev loop or first download. That's something that our developers need to understand (and experience).

AFAIK, DR doesn't want anything to adopt AOT OOB.

Correct, it's not good as default, you need to know what you are doing.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 8, 2024

@danroth27 must address it. It's not really a 🦖 call. Dan, see the last few remarks.

@guardrex
Copy link
Collaborator Author

@maraf ... Do you want to review this?

@danroth27 ... Do you want to reverse your earlier decision on not enabling AOT OOB for one or more of our WASM sample apps? Even if you do want to reverse that decision, it would only be for the WASMBrowserAppImportExportInterop sample on this PR. If you want to reverse the decision for the other WASM samps, I'll open a new issue to work on them (and their coverage) separately.

@danroth27
Copy link
Member

@danroth27 ... Do you want to reverse your earlier decision on not enabling AOT OOB for one or more of our WASM sample apps? Even if you do want to reverse that decision, it would only be for the WASMBrowserAppImportExportInterop sample on this PR. If you want to reverse the decision for the other WASM samps, I'll open a new issue to work on them (and their coverage) separately.

I don't see a need to have a sample for AOT given that the only code change needed to enable AOT is setting the RunAOTCompilation property.

@guardrex
Copy link
Collaborator Author

Thanks, @danroth27.

I'm just waiting on feedback from @maraf. I think we can merge this after his review/updates.

@guardrex guardrex merged commit cbe3b62 into main Aug 22, 2024
3 checks passed
@guardrex guardrex deleted the guardrex/blazor-import-export-interop-updates branch August 22, 2024 13:37
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.

Additional JSImport/JSExport Examples
7 participants