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

Switch to use ICU4X #335

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

Conversation

zbraniecki
Copy link
Collaborator

Fixes #329

@zbraniecki
Copy link
Collaborator Author

No performance impact from the switch on any of the fluent-bundle perf tests.

@zbraniecki
Copy link
Collaborator Author

@dminor @aethanyc @makotokato - I'd like to ask for your architectural advice.

Historically, fluent-bundle was using intl_pluralrules which contained all CLDR locales baked in. With this switch we now use ICU4X, and in this PR so far I'm just using PluralRules::try_new which uses compiled data.

We will also want to add DateTimeFormat and NumberFormat, and all three are transitive. They are created based on FluentBundle and memoized using intl-memoizer.

My understanding is that for Mozilla use case, you'd prefer to create each component instance using custom DataProvider, so the shift here is that we'd like to have an option to pass such DataProvider instance to the FluentBundle constructor, and have it pass it to PluralRules/DateTimeFormat/NumberFormat etc. constructor.
Is that accurate?

If so, this will require also changes to `L10nRegistry as this is where FluentBundle is generated.

@waywardmonkeys
Copy link
Collaborator

I have a pile of pending PRs here in this repo, one of which, #332, fixes the CI failure that you see here.

@alerque alerque added the icu4x Migrate example components to use icu4x instead of custom stubbed out components. label May 6, 2024
@alerque alerque mentioned this pull request May 6, 2024
9 tasks
@alerque
Copy link
Collaborator

alerque commented May 7, 2024

I'm going to rebase this on main to resolve the merge conflicts so it's that much easier to work on and build on. That way if somebody branches from this to keep working on it it should be able to integrate once things start coming together. In the process I'm running rustfmt and clippy --fix on the commits before rebasing as this pre-empts most of the merge conflicts that have cropped up. I'll manually take care of the ones that remain.

If by chance you have newer work on this branch than what is pushed here do feel free to force push and clobber my rebase. I can always redo it, but it would likely be easier to redo from scratch with my git rerere cache than for you to try to rebase on what I'm about to force push.

@alerque
Copy link
Collaborator

alerque commented May 7, 2024

@zbraniecki One thing I noticed during the rebase is that the refactoring of intl-memoizer just blew away the inline documentation. I presume that was on purpose and because you think the docs would need to be overhauled, but there were a few documentation fixes in the main branch that could not be re-applied because there were no docs to apply them to. If and when you think docs are ready to be brought back I can maybe help with fitting some of the old pieces/changes into the new locations. For reference git diff a2cef6b..c904ac3 intl-memoizer is the relevant range of changes to the default branch since this was branched and of course a2cef6b..0e1b2e5 is the original range of commits you had here before I rebased them just now.

@alerque
Copy link
Collaborator

alerque commented May 7, 2024

It looks like I goofed up some of the workspace related changes a little in the rebase, straightening that out now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icu4x Migrate example components to use icu4x instead of custom stubbed out components.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using or switching to icu4x crates?
3 participants