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

Disable dotnet.js fingerprinting for .NET 8 #31829

Merged
merged 8 commits into from
May 29, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Apr 17, 2023

  • Turn off fingerprinting dotnet.js for .NET 8
  • Fix definition of static web asset for dotnet.js without fingerprint

@maraf maraf added the Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch label Apr 17, 2023
@maraf maraf self-assigned this Apr 17, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Apr 17, 2023
@javiercn
Copy link
Member

@maraf can you give me more context on the reason for this?

I believe when we AoT we end up producing a file that is tailored to a given app. Won't we want to fingerprint that?

@maraf
Copy link
Member Author

maraf commented Apr 18, 2023

@maraf can you give me more context on the reason for this?

It is related to this client side change dotnet/aspnetcore#47752.

As discussed offline, when the runtime loads boot config, we don't have a way to know the fingerprint of dotnet.js in blazor.webassembly.js, until we introduce build-time generation of importmap or html/js token rewrite.

EDIT: Users can still use startupOptions to provide a loadBootResource function which can modify the URL of dotnet.js

@javiercn
Copy link
Member

@maraf thanks for the additional details.

We don't bundle blazor.webassembly.js and dotnet.js together, do we?

https://github.com/dotnet/aspnetcore/blob/main/src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts#L15 is only importing the types, isn't it?

From what I can tell here that is the case.

Here is where it is being resolved:
https://github.com/dotnet/aspnetcore/blob/main/src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts#L174

Is there any reason that will prevent us loading dotnet.js in the way it's currently done?

@maraf
Copy link
Member Author

maraf commented Apr 18, 2023

We don't bundle blazor.webassembly.js with dotnet.js. At least at the moment. One of the shapes we are investigating is spliting dotnet.js into a small loader + wasm feature detector and the rest (mostly emscripten code). In such shape we would probably want embed the boot config into the loader (it's going to be really small piece of code). With some configuration, we might consider embeding the loader into the blazor.webassembly.js.

For scenario with memory snapshot (all resources bundled into the wasm file), there won't be any boot config, because the runtime will be already initialized.

For these reasons it's important that we control how boot config is loaded. And that implies that blazor.webassembly.js needs to know where dotnet.js is without first loading boot config.

Is it now clearer? I'm happy to explain in more details.

@maraf maraf marked this pull request as ready for review April 18, 2023 09:55
@maraf maraf requested review from lewing, radical, pavelsavara and a team as code owners April 18, 2023 09:55
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.

LGTM

@javiercn
Copy link
Member

For scenario with memory snapshot (all resources bundled into the wasm file), there won't be any boot config, because the runtime will be already initialized.

This would be an opt-in, won't it?

For these reasons it's important that we control how boot config is loaded. And that implies that blazor.webassembly.js needs to know where dotnet.js is without first loading boot config.

Is there any issue if we download it twice? I assume if we embed this data, it will be identical to the file we produce during the build, won't it? Alternatively,

We don't bundle blazor.webassembly.js with dotnet.js. At least the moment. One of the shapes we are investigating is spliting dotnet.js into a small loader + wasm feature detector and the rest (mostly emscripten code). In such shape we would probably want to embed the boot config into the loader (it's going to be really small piece of code). With some configuration, we might consider embeding the loader into the blazor.webassembly.js.

I think this warrants a wider discussion. I do not believe dotnet.js should be imposing any requirement on consumers around bundling/layout. Boot.Webassembly.js should be free to do a dynamic import for dotnet.js from whatever location it deems appropriate and to bundle the resources at it sees fit, how the payloads are split is an app decision, not a framework decision.

This is even more important in the case of Blazor United, as we might choose to bring the webassembly bits on demand.

Could we instead start by turning it off explicitly in the project? We can later on add the default condition, isn't it?

It will help a lot to get everybody in the same page if we can have a doc/spec somewhere in an issue in aspnetcore or runtime with a list of scenarios and how we expect them to work. As sample scenarios (probably not exhaustive):

  • Built/Published app without linking nor AoT.
  • Built/Published app with linking (no AoT).
  • Built/Published app with linking and AoT.
  • Loader+MemorySnapshot.

It'll be good to have a clear picture of:

  1. Deployment layout on each scenario (what files are produced).
  2. What is the boot sequence for that mode.

@javiercn
Copy link
Member

As discussed offline, when the runtime loads boot config, we don't have a way to know the fingerprint of dotnet.js in blazor.webassembly.js, until we introduce build-time generation of importmap or html/js token rewrite.

EDIT: Users can still use startupOptions to provide a loadBootResource function which can modify the URL of dotnet.js

I don't think we have a clear solution for this problem just yet. Even if importmap and html/js token rewrite are potential candidates, it's not clear to me that they don't present challenges, for example with regards to the PWA support.

In addition to that, even if a user can provide a custom URL via startupOptions, the experience matters too, and this change is regressing that experience.

At the moment, you get the fingerprinted asset as the default experience. With the changes we are trying to make, the experience is that we get the non-fingerprinted version and turning it on requires you to do a non-trivial amount of setup.

I suspect the issue here is that there is a chicken and egg problem here, isn't it? You can't put a tagged version of dotnet.js (or any asset for that matter) inside the config if you want to embed the config in the asset.

@maraf
Copy link
Member Author

maraf commented Apr 18, 2023

All of the bundling scenarios I mentioned are opt-in. They come from various requests we have collected. Here are some details dotnet/runtime#70762. In one of our calls we have discussed these, but I should have written them down to an issue. Also, I didn't realized this change is needed until last week.

I suspect the issue here is that there is a chicken and egg problem here, isn't it? You can't put a tagged version of dotnet.js (or any asset for that matter) inside the config if you want to embed the config in the asset.

I don't think embedding is the problem here. The problem is that we don't have an end-to-end solution. At the moment, blazor.webassembly.js is the fixed file that is not fingerprinted, which probably may cause the same caching problems when the app updates blazor version, right? The change we are proposing makes two such problematic assets.

the experience matters too, and this change is regressing that experience.

Yes, that is absolutely right. I don't have an as good solution as it is now (without importmap / token rewrite). We can introduce an easier way to define fingerprint, for example with simple extra attribute on blazor.webassembly.js script tag, or something similar.

Is there any issue if we download it twice
We can download it twice, at least for now, but I think we shouldn't end with such solution.

EDIT

This is even more important in the case of Blazor United, as we might choose to bring the webassembly bits on demand.

I don't think any of proposal interfere with bringing wasm on demand. If configured, the only overhead would be inlined config, but that is opt-in feature.

Even if importmap and html/js token rewrite are potential candidates, it's not clear to me that they don't present challenges, for example with regards to the PWA support.

Do you have something concrete on your mind? My expectations are based on how webpack works and that is friendly with pwa.

@maraf
Copy link
Member Author

maraf commented Apr 18, 2023

Could we turn off fingerprinting for preview4 and continue with exploring possible final solution in the upcoming period?

@maraf
Copy link
Member Author

maraf commented Apr 19, 2023

I'm going to close this one, as for preview4 we are going to do double boot config download.

@maraf maraf closed this Apr 19, 2023
@maraf
Copy link
Member Author

maraf commented May 19, 2023

@javiercn We have landed the change how dotnet.js is composed README.md#es6-modules.
There is no need for fingerprinting the entry dotnet.js

@maraf maraf reopened this May 19, 2023
# Conflicts:
#	src/WasmSdk/Tasks/ComputeWasmBuildAssets.cs
@lewing lewing requested a review from javiercn May 23, 2023 16:14
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

I still want us to discuss some things in this area, since it's adding more files to the download cascade and can potentially be problematic with version-to-version updates (for example patches, or major updates).

It is still valuable if we leave the boot.config.json file on the wwwroot folder so that blazor can find the fingerprinted version if it needs to.

Let's go with this right now and we can try to address this in Preview6/7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants