[Merged by Bors] - Integrate ICU4X into Intl module
#2083
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedel1043
added
enhancement
|
Slightly related to #2068, since CLDR data increases our binary size significantly, and some users won't even use Edit: fixed by the |
Test262 conformance changesVM implementation
Fixed tests (22):Broken tests (2): |
Benchmark for e62ae17Click to view benchmark
|
|
Opened unicode-org/icu4x#1925 to fix the two failing tests. |
Codecov Report
@@ Coverage Diff @@
## main #2083 +/- ##
==========================================
- Coverage 43.86% 43.82% -0.04%
==========================================
Files 216 217 +1
Lines 19575 19559 -16
==========================================
- Hits 8586 8572 -14
+ Misses 10989 10987 -2
Continue to review full report at Codecov.
|
Benchmark for 92e463dClick to view benchmark
|
|
Do we really have to include the data in the executable? In addition to increasing binary sizes, this also is problematic for deterministic builds. We call Can we not have an api for including the data? It would then most likely be a field on the |
Yeah, we do. The alternative would be to download the CLDR data at runtime, but that implies giving boa networking access at runtime.
I think the biggest disadvantage of this would be that a user that wants to use the If we want to preserve deterministic builds, and want the ability to include the data only on tests, a feature gate that also skips running the |
|
In addition to the feature flag, what do you think about hosting the |
|
By hosting you mean pushing the generated file to this repo? That would increase the size of this repo. Its current size is approximately 10 MB, and by additionally hosting the postcard file, we would increase its size to 43MB. In addition to that, I'm thinking about the mainteinance costs of hosting the postcard. If we did that, everyone would need to remember to update the file when a new Also, I'm planning on replacing this current solution with an auto-generated module using the new |
|
My idea would have been to generate the postcard files for every of our releases and push them to a separate branch or even have them in a separate git repository. That way we would sidestep the comparability and the size issue. But apart from that we still would need a build.rs to download the files anyways. I think we should cut down on the new dependencies for that. Is there a need for reqwest = { version = "0.11.10", default-features = false, features = ["rustls-tls"]}
tokio = { version = "1", features = ["full"] }
zip = { version = "0.6.2", default-features = false, features = ["deflate"] }Imo the few lines of code that we save are not worth the ~50 additional dependencies. |
|
Yeah, I was also thinking using |
jedel1043
force-pushed
the
icu4x-integration
branch
from
2d10067 to
9ffdfd3
Compare
|
OK, following @raskad suggestions and seeing v8 and spidermonkey made the same tradeoff about data providers, I completely removed the This is the most flexible option we currently can offer to our users, and generating the data is not that difficult with the I also took advantage of the |
Benchmark for 5121fcaClick to view benchmark
|
Benchmark for a78fb74Click to view benchmark
|
Benchmark for 235a5ccClick to view benchmark
|
Benchmark for a9f220eClick to view benchmark
|
raskad
approved these changes
May 27, 2022
There was a problem hiding this comment.
This approach looks really good to me. I would be happy to merge this. Two follow ups I see are:
- creation of an example on how to include the icu data (can probably wait until we have a good amount of
Intltests passing so that the feature can be used) - Usage of icu data in our tests
Razican
approved these changes
May 29, 2022
Benchmark for b020856Click to view benchmark
|
|
@Razican Sorry! I addressed your comments and requested a review but I didn't see that you'd already approved the PR 😅 |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 30, 2022
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel neccesary. ---> This Pull Request integrates an `ICU4X` data provider API in our codebase, to make use of the internationalization APIs that this crate provides. It changes the following: - Creates an API for pluggable icu data providers at `Context` creation, adding an `Icu` struct to store the provider (and some other internationalization tools) at runtime. - Slightly changes locale related functions to preserve the `Locale` type and ensure correctness. (Will make some other changes related to this). - Integrates the `sys_locale` crate to fetch the current default locale of an user instead of always returning `en-US`.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Intl moduleIntl module
Razican
added a commit
that referenced
this pull request
Jun 8, 2022
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel neccesary. ---> This Pull Request integrates an `ICU4X` data provider API in our codebase, to make use of the internationalization APIs that this crate provides. It changes the following: - Creates an API for pluggable icu data providers at `Context` creation, adding an `Icu` struct to store the provider (and some other internationalization tools) at runtime. - Slightly changes locale related functions to preserve the `Locale` type and ensure correctness. (Will make some other changes related to this). - Integrates the `sys_locale` crate to fetch the current default locale of an user instead of always returning `en-US`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request integrates an
ICU4Xdata provider API in our codebase, to make use of the internationalization APIs that this crate provides.It changes the following:
Contextcreation, adding anIcustruct to store the provider (and some other internationalization tools) at runtime.Localetype and ensure correctness. (Will make some other changes related to this).sys_localecrate to fetch the current default locale of an user instead of always returningen-US.