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

[browser] Separate hybrid globalization JS code to es6 module #101543

Merged
merged 33 commits into from
May 17, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Apr 25, 2024

Contributes to #98483.

  • separate hybrid globalization JS code into a separate module
  • add the module if HybridGlobalization=true

dotnet.runtime.js without this PR: 202k
dotnet.runtime.js with this PR: 191k

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm labels Apr 25, 2024
@ilonatommy ilonatommy self-assigned this Apr 25, 2024
@ilonatommy ilonatommy marked this pull request as draft April 25, 2024 13:20
@maraf
Copy link
Member

maraf commented Apr 25, 2024

When solved, is there a good way to read MsBuild's value and attach/remove module in the runtime? The ideal solution should avoid creating a new runtime flavor.

Sure. We will update GenerateWasmBootJson (and WasmAppBuilder for the time being) to either include the module or not, and update the loading in TS

@ilonatommy ilonatommy marked this pull request as ready for review May 14, 2024 14:51
@ilonatommy
Copy link
Member Author

ilonatommy commented May 14, 2024

Blazor-connected failures in WBT require changes to https://github.com/ilonatommy/aspnetcore/blob/3e168fe85e2330621ed12d9c9524c80e0f743dc6/src/Components/dotnet-runtime-js/dotnet.d.ts#L176 (adding jsModuleGlobalization).

Wasm.Build.Tests.Blazor.IcuTests.HybridWithInvariant(config: "Debug", invariant: False) [FAIL]
      Could not find dotnet.globalization.js in bundle directory: C:\helix\work\workitem\e\wbt artifacts\blz_hybrid_Debug_4lfnum1h_dzk\bin\Debug\net9.0\wwwroot\_framework. Actual files on disk: dotnet.js, dotnet.js.map, dotnet.native.js, dotnet.native.wasm, dotnet.runtime.js, dotnet.runtime.js.map
      Stack Trace:
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(176,0): at Wasm.Build.Tests.ProjectProviderBase.AssertDotNetFilesSet(IReadOnlySet`1 expected, IReadOnlyDictionary`2 superSet, IReadOnlyDictionary`2 actualReadOnly, Boolean expectFingerprintOnDotnetJs, String bundleDir)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(158,0): at Wasm.Build.Tests.ProjectProviderBase.FindAndAssertDotnetFiles(String binFrameworkDir, Boolean expectFingerprintOnDotnetJs, IReadOnlyDictionary`2 superSet, IReadOnlySet`1 expected)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(78,0): at Wasm.Build.Tests.ProjectProviderBase.FindAndAssertDotnetFiles(AssertBundleOptionsBase assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(44,0): at Wasm.Build.Tests.ProjectProviderBase.AssertBasicBundle(AssertBundleOptionsBase assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/WasmSdkBasedProjectProvider.cs(83,0): at Wasm.Build.Tests.WasmSdkBasedProjectProvider.AssertBundle(AssertWasmSdkBundleOptions assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmProjectProvider.cs(17,0): at Wasm.Build.Tests.BlazorWasmProjectProvider.AssertBundle(BlazorBuildOptions options)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmTestBase.cs(128,0): at Wasm.Build.Tests.BlazorWasmTestBase.AssertBundle(String buildOutput, BlazorBuildOptions blazorBuildOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmTestBase.cs(67,0): at Wasm.Build.Tests.BlazorWasmTestBase.BlazorBuild(BlazorBuildOptions options, String[] extraArgs)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/IcuTests.cs(38,0): at Wasm.Build.Tests.Blazor.IcuTests.HybridWithInvariant(String config, Nullable`1 invariant)
        --- End of stack trace from previous location ---

Blocked by flow of dotnet/aspnetcore#55716 to runtime.

@pavelsavara
Copy link
Member

Blazor-connected failures in WBT require changes to https://github.com/ilonatommy/aspnetcore/blob/3e168fe85e2330621ed12d9c9524c80e0f743dc6/src/Components/dotnet-runtime-js/dotnet.d.ts#L176 (adding jsModuleGlobalization).

I think that the blazor PR would not fix it because it's just type definition. The error is complaining about missing files. Likely flow of blazor with this change will fix it.

cc @maraf

@maraf
Copy link
Member

maraf commented May 14, 2024

Blazor-connected failures in WBT require changes to https://github.com/ilonatommy/aspnetcore/blob/3e168fe85e2330621ed12d9c9524c80e0f743dc6/src/Components/dotnet-runtime-js/dotnet.d.ts#L176 (adding jsModuleGlobalization).

Wasm.Build.Tests.Blazor.IcuTests.HybridWithInvariant(config: "Debug", invariant: False) [FAIL]
      Could not find dotnet.globalization.js in bundle directory: C:\helix\work\workitem\e\wbt artifacts\blz_hybrid_Debug_4lfnum1h_dzk\bin\Debug\net9.0\wwwroot\_framework. Actual files on disk: dotnet.js, dotnet.js.map, dotnet.native.js, dotnet.native.wasm, dotnet.runtime.js, dotnet.runtime.js.map
      Stack Trace:
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(176,0): at Wasm.Build.Tests.ProjectProviderBase.AssertDotNetFilesSet(IReadOnlySet`1 expected, IReadOnlyDictionary`2 superSet, IReadOnlyDictionary`2 actualReadOnly, Boolean expectFingerprintOnDotnetJs, String bundleDir)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(158,0): at Wasm.Build.Tests.ProjectProviderBase.FindAndAssertDotnetFiles(String binFrameworkDir, Boolean expectFingerprintOnDotnetJs, IReadOnlyDictionary`2 superSet, IReadOnlySet`1 expected)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(78,0): at Wasm.Build.Tests.ProjectProviderBase.FindAndAssertDotnetFiles(AssertBundleOptionsBase assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs(44,0): at Wasm.Build.Tests.ProjectProviderBase.AssertBasicBundle(AssertBundleOptionsBase assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/WasmSdkBasedProjectProvider.cs(83,0): at Wasm.Build.Tests.WasmSdkBasedProjectProvider.AssertBundle(AssertWasmSdkBundleOptions assertOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmProjectProvider.cs(17,0): at Wasm.Build.Tests.BlazorWasmProjectProvider.AssertBundle(BlazorBuildOptions options)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmTestBase.cs(128,0): at Wasm.Build.Tests.BlazorWasmTestBase.AssertBundle(String buildOutput, BlazorBuildOptions blazorBuildOptions)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmTestBase.cs(67,0): at Wasm.Build.Tests.BlazorWasmTestBase.BlazorBuild(BlazorBuildOptions options, String[] extraArgs)
        /_/src/mono/wasm/Wasm.Build.Tests/Blazor/IcuTests.cs(38,0): at Wasm.Build.Tests.Blazor.IcuTests.HybridWithInvariant(String config, Nullable`1 invariant)
        --- End of stack trace from previous location ---

Blocked by flow of dotnet/aspnetcore#55716 to runtime.

This is just build setup error, isn't it? The globalization.js file is not copied to the output folder. Either the SDK doesn't correctly work or the WBT check isn't cotrect. There shouldn't be anything needed on blazor side to make it work.

The dotnet.d.ts in aspnetcore repo is only important for their build of their typescript that interact with runtime API.

@ilonatommy
Copy link
Member Author

This is just build setup error, isn't it? The globalization.js file is not copied to the output folder. Either the SDK doesn't correctly work or the WBT check isn't cotrect. There shouldn't be anything needed on blazor side to make it work.

The dotnet.d.ts in aspnetcore repo is only important for their build of their typescript that interact with runtime API.

WBT works correctly (non-Blazor IcuTests pass). You're right, it fails because WBT expects the globalization module to be in the bundle when HybridGlobalization == true. Not sure if we can rely on the changes to flow and the problem to "fix itself" as Pavel mentioned.
However, even if the file was copied, we have no entry in blazor.boot.json about jsModuleGlobalization (added by this PR) that is needed to initialize the module loading. Again, no idea how to check if this will get fixed automatically.

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

ilonatommy commented May 17, 2024

Firefox failures are #101617 (locally passes, it's only a CI problem), HybridGlobalization_AOT failures are: #102373 - fails on main as well.

@ilonatommy ilonatommy merged commit 43b7b53 into dotnet:main May 17, 2024
163 of 173 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…#101543)

* dotnet.hybrid.js module creation when MT is off

* Importing module

* Remove globalization code from main module, create the globalization module for ST runtime.

* Fix: `runtimeHelper` is not null if we cache it on module init.

* Export functions used by hybrid globalization only when HG is on.

* Do not try to call when methods are not initialized.

* `mono_wasm_get_locale_info` is used also in non-HG mode, add it to the main module.

* Check WBT for presence of globalization module if HG is switched on.

* Fix MT globalization tests - NativeName is fixed there.

* Globalization module is not expected to change on relink.

* Fix blazor's assets.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants