facebook / hermes

A JavaScript engine optimized for running React Native.
https://hermesengine.dev/
MIT License
9.41k stars 596 forks source link

Implement `getCanonicalLocales` in non apple/android environments #1361

Closed manuelpuyol closed 1 month ago

manuelpuyol commented 1 month ago

Related to https://github.com/facebook/hermes/issues/1350 and https://github.com/facebook/hermes/discussions/1211

Summary

As an initial step for Intl backed by ICU, I'm implementing the getCanonicalLocales function using BCP47Parser. The implementation is exactly the same as Apple's implementation

Test Plan

This change should be covered by the get-canonical-locales.js test.

manuelpuyol commented 1 month ago

@neildhar looks like the base images used for android and emscripten only have ICU 66, but I expected them to skip the ICU library from https://github.com/facebook/hermes/blob/d62b96ea44dd8850cd5f6e26d2de842177dc7c22/CMakeLists.txt#L515-L519

neildhar commented 1 month ago

Hey @manuelpuyol, thanks for putting this together! The main high level suggestion I have here is to use the existing unicode tag parsing and canonicalisation functionality we use for the Apple Intl implementation. In particular, take a look at BCP47Parser.cpp, and how it is used in our existing Apple implementation in PlatformIntlApple.mm. You may be able to just copy the implementation over from the Apple platform. (we're planning to unify these in the longer term)

That should also solve the ICU issue you shared, since all the tag parsing and canonicalisation will be done by us.

manuelpuyol commented 1 month ago

hey @neildhar! I used Apple's implementation and reverted the ICU change. I'm not sure what is going on with test-e2e though, compileReleaseKotlin is failing but doesn't look related to my changes and I see it failing on other PRs

facebook-github-bot commented 1 month ago

@neildhar has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 month ago

@manuelpuyol has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 1 month ago

@neildhar merged this pull request in facebook/hermes@ef5fa1a2daf92ce93c1124224c8b96b97f5d98c9.