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

WASM Demo #4684

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

WASM Demo #4684

wants to merge 6 commits into from

Conversation

ashu26jha
Copy link
Contributor

@ashu26jha ashu26jha commented Mar 14, 2024

This Pull Request is intended to shift our WASM demo from using create_compiled to dynamically load locales whenever request from a front end. It achieves falling task:

  1. Created a DataProviderManager which forks locales on the go

  2. Added fallback with config

  3. It prints the fallback locale, also printing trying possibilties

    • Instead of logging in the console, we display onto the frontend if. It could a better user experence. This needs approval.

Fixes #2769

Also we could also discuss the possiblity of adding DataGen in our FFI declarations, I could only find it for Dart, not for C++ and JavaScript. I could work on this, if this is what dev wants. This could be useful reduce application's binary size, for eg.

  1. Retrieve user's system locale.
  2. Generate the data dynamically for that locale only.

@ashu26jha ashu26jha requested a review from a team as a code owner March 14, 2024 05:23
@ashu26jha ashu26jha changed the title WASM refactoring WASM Demo Mar 14, 2024
@sffc sffc requested review from sffc and robertbastian March 14, 2024 14:40
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

let fallbackerWithConfig = fallbacker.for_config(fallbackConfig);

this.fallbacker = fallbackerWithConfig;
enProvider.enable_locale_fallback_with(this.fallbacker);
Copy link
Member

Choose a reason for hiding this comment

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

Issue: enable_locale_fallback_with should take a ICU4XLocaleFallbacker, not a ICU4XLocaleFallbackerWithConfig. I think TypeScript should catch this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I wanted to display the fallbacked locale. ICU4XLocaleFallbackerWithConfig gives an iterator which helps to get the fallback, not sure how to achieve this using ICU4XLocaleFallbacker.

Also it doesn't through an error 😕

Copy link
Member

Choose a reason for hiding this comment

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

The resolved locale is returned in DataResponseMetdata.locale (None if it's the request locale).

Copy link
Member

Choose a reason for hiding this comment

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

(oh this is not Rust, ignore my comment).

docs/tutorials/npm/src/ts/data-provider-manager.ts Outdated Show resolved Hide resolved
docs/tutorials/npm/src/ts/date-time.ts Outdated Show resolved Hide resolved
docs/tutorials/npm/gen.sh Outdated Show resolved Hide resolved
docs/tutorials/npm/gen.sh Outdated Show resolved Hide resolved
docs/tutorials/npm/package.json Outdated Show resolved Hide resolved
docs/tutorials/npm/gen.sh Outdated Show resolved Hide resolved
@ashu26jha ashu26jha marked this pull request as draft March 22, 2024 02:31
@ashu26jha
Copy link
Contributor Author

ashu26jha commented Apr 10, 2024

Just to test the new fallbacker instead of ICU4XLocaleFallbackerWithConfig. I tried to write this function, but couldn't create the formatter:

let enProvider = await createDataProviderFromBlob('dist/en.postcard');
let unProvider = await createDataProviderFromBlob('dist/und.postcard');
let fallbacker = ICU4XLocaleFallbacker.create(unProvider);
enProvider.enable_locale_fallback_with(fallbacker);
const locale = result(() => ICU4XLocale.create_from_string("bn"));
const groupingStrategy = ICU4XFixedDecimalGroupingStrategy.Auto;
const formatter = result(() => ICU4XFixedDecimalFormatter.create_with_grouping_strategy(
    enProvider,
    unwrap(locale),
    groupingStrategy
));

Also @robertbastian for caching we could possible generate a hash of gen.sh, If this hash is not changed no need to generate files again. This way we will avoid repeated use of datagen.

const enFilePath = 'dist/en.postcard';
let enProvider = await this.createDataProviderFromBlob(enFilePath);
this.loadedLocales.add(ICU4XLocale.create_from_string("en"));
const unFilePath = 'dist/en.postcard';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried this with dist/und.postcard, with no help. Will fix this in consecutive commits**

@ashu26jha
Copy link
Contributor Author

Added changes suggested by @sffc & @robertbastian except the fallbacker. I have added a sample code of how I am planning to integrate. Is this the correct way to integrate fallbacker?

@sffc
Copy link
Member

sffc commented Apr 23, 2024

ICU4X WG discussion:

  • @robertbastian - For splitting locales, this seems reasonable. However, ideally we could also split functionality, but that might result in more duplication.
  • @robertbastian - The data provider manager that is being implemented might better live in the FFI crate. We could upstream it later. Might need callbacks.

Conclusion: yes, merge this PR after comments have been resolved.

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.

WASM demo takes 40 seconds to load and is not interactive
3 participants