Skip to content

Merged PR 9371: Revive support for globalization and localization in Blazor WASM #24773

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 18, 2020
Merged

Merged PR 9371: Revive support for globalization and localization in Blazor WASM #24773

merged 2 commits into from
Aug 18, 2020

Conversation

pranavkm
Copy link
Contributor

Manually porting preview8 changes to master

Revive support for globalization and localization in Blazor WASM

  • Load icu and timezone data files
  • Unskip tests

Fixes #24174
Fixes #22975
Fixes #23260

@pranavkm pranavkm requested review from SteveSandersonMS and a team as code owners August 11, 2020 04:48
@pranavkm pranavkm added the area-blazor Includes: Blazor, Razor Components label Aug 11, 2020
captainsafia
captainsafia reviewed Aug 11, 2020
View reviewed changes
src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts
@@ -239,14 +239,26 @@ function createEmscriptenModuleInstance(resourceLoader: WebAssemblyResourceLoade
/* hash */ resourceLoader.bootConfig.resources.runtime[dotnetWasmResourceName],
/* type */ 'dotnetwasm');

const dotnetTimeZoneResourceName = 'dotnet.timezones.dat';
const dotnetTimeZoneResourceName = 'dotnet.timezones.blat';
Copy link
Member

Choose a reason for hiding this comment

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

Is this a typo or a legit file format?

Copy link
Member

Choose a reason for hiding this comment

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

I've heard Larry talk about this so I guess it's the name of the archive file format. Apparently it's like a tar file.

captainsafia
captainsafia reviewed Aug 11, 2020
View reviewed changes
src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts
let timeZone = "UTC";
try {
timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone;
} catch { }
Copy link
Member

Choose a reason for hiding this comment

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

Why is it OK to ignore errors thrown here?

captainsafia
captainsafia reviewed Aug 11, 2020
View reviewed changes
src/Components/WebAssembly/Sdk/integrationtests/WasmBuildIntegrationTest.cs
@@ -192,10 +193,10 @@ public async Task Build_WithBlazorEnableTimeZoneSupportDisabled_DoesNotCopyTimeZ

var runtime = bootJsonData.resources.runtime.Keys;
Assert.Contains("dotnet.wasm", runtime);
Assert.DoesNotContain("dotnet.timezones.dat", runtime);
Assert.DoesNotContain("dotnet.timezones.blat", runtime);
Copy link
Member

Choose a reason for hiding this comment

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

Ah -- OK. So the blat file extension is intentional...

captainsafia
captainsafia reviewed Aug 11, 2020
View reviewed changes
src/Components/WebAssembly/WebAssembly/src/Hosting/SatelliteResourcesLoader.cs
@@ -55,7 +57,8 @@ public virtual async ValueTask LoadCurrentCultureResourcesAsync()

for (var i = 0; i < assemblies.Length; i++)
{
Assembly.Load((byte[])assemblies[i]);
using var stream = new MemoryStream((byte[])assemblies[i]);
Copy link
Member

Choose a reason for hiding this comment

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

👍

SteveSandersonMS
SteveSandersonMS approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Seems like this is mostly porting pre-existing changes so approving. Looks good anyway.

@pranavkm pranavkm force-pushed the prkrishn/revive-globalization branch from c3df503 to 3818ef0 Compare August 11, 2020 18:00
@pranavkm
Copy link
Contributor Author

@lewing I'm seeing issues with non-en cultures. Here's the stack trace:

[email protected]:1
ves_icall_System_Environment_FailFast@00aba79e:1
ves_icall_System_Environment_FailFast_raw@00aba79e:1
do_icall@00aba79e:1
do_icall_wrapper@00aba79e:1
interp_exec_method@00aba79e:1
interp_runtime_invoke@00aba79e:1
mono_jit_runtime_invoke@00aba79e:1
do_runtime_invoke@00aba79e:1
mono_runtime_try_invoke@00aba79e:1
mono_runtime_try_invoke_handle@00aba79e:1
invoke_resolve_method@00aba79e:1
mono_alc_invoke_resolve_using_resolve_satellite@00aba79e:1
mono_alc_invoke_resolve_using_resolve_satellite_nofail@00aba79e:1
netcore_load_reference@00aba79e:1
mono_assembly_request_byname@00aba79e:1
ves_icall_System_Reflection_Assembly_InternalLoad@00aba79e:1
ves_icall_System_Reflection_Assembly_InternalLoad_raw@00aba79e:1
do_icall@00aba79e:1
do_icall_wrapper@00aba79e:1
interp_exec_method@00aba79e:1
interp_runtime_invoke@00aba79e:1
mono_jit_runtime_invoke@00aba79e:1
do_runtime_invoke@00aba79e:1
mono_runtime_try_invoke@00aba79e:1
mono_runtime_try_invoke_handle@00aba79e:1
invoke_resolve_method@00aba79e:1
mono_alc_invoke_resolve_using_resolve_satellite@00aba79e:1

Could you help?

@pranavkm pranavkm added the blocked The work on this issue is blocked due to some dependency label Aug 11, 2020
@pranavkm pranavkm added this to the 5.0.0-rc1 milestone Aug 11, 2020
@CoffeeFlux
Copy link

I'll take a look; getting this set up locally.

@CoffeeFlux
Copy link

Finally got a repro working, looking into this now.

@CoffeeFlux CoffeeFlux mentioned this pull request Aug 14, 2020
Merged
@CoffeeFlux
Copy link

Building locally with the linker disabled and swapping out SPC for one built with dotnet/runtime#40817 seems to fix this locally, so hopefully once that change flows down to this repo we'll be good to go. 😄

@benaadams
Copy link
Member

ooof
image

@pranavkm
Merged PR 9371: Revive support for globalization and localization in …
4e57d1b 
…Blazor WASM

Revive support for globalization and localization in Blazor WASM

* Load icu and timezone data files
* Unskip tests

Fixes #24174
Fixes #22975
Fixes #23260
@pranavkm pranavkm force-pushed the prkrishn/revive-globalization branch from 3818ef0 to 4e57d1b Compare August 17, 2020 16:02
@pranavkm pranavkm added auto-merge and removed blocked The work on this issue is blocked due to some dependency labels Aug 17, 2020
@ghost
Copy link

ghost commented Aug 17, 2020

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@pranavkm
Copy link
Contributor Author

@benaadams we're looking at sharding the icu file, which should make the default compressed size about half of the numbers being reported here. There's also an option to enable invariant mode if you do not require this data that should eliminate this file entirely.

@pranavkm pranavkm changed the base branch from master to release/5.0 August 17, 2020 21:24
@pranavkm pranavkm merged commit 2ad1b6d into release/5.0 Aug 18, 2020
@pranavkm pranavkm deleted the prkrishn/revive-globalization branch August 18, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@captainsafia captainsafia captainsafia approved these changes

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
5.0.0-rc1
Development

Successfully merging this pull request may close these issues.

Enable ICU in blazor Revive Blazor Wasm globalization \ localization tests Figure out how timezones work in browser-wasm
5 participants
@pranavkm @CoffeeFlux @benaadams @SteveSandersonMS @captainsafia