facebook
Improve ICU4C-based Intl.DateTimeFormat implementation
Summary
Improve and refactor ICU4C-based Intl.DateTimeFormat implementation for non-Android / non-Apple platforms.
- Add a flag in top-level CMakeLists.txt for enabling Intl formatRange() and formatRangeToParts() APIs for non-Android and non-Apple platforms, i.e. Linux / ICU4C-backed Intl implementation.
- Add the platform plumbing code for formatRange() and formatRangeToParts().
- Remove PlatformIntlShared.h/.cpp as the remaining logic is only used in ICU4C-backed Intl.DateTimeFormat implementation and is now all inside impl_icu/DateTimeFormat.cpp.
- Fix a bug in BCP47Parser isUnicodeVariantSubtag() so that the code accounts for the "digit" porition in the "digit alphanum{3}" syntax definition.
- impl_icu/DateTimeFormat.h|.cpp is the implementation of Intl.DateTimeFormat using ICU4C C APIs. No ICU4C C++ APIs are used for ABI stability.
- In PlatformIntlICU.cpp, remove DateTimeFormat implementation code and redirect DateTimeFormat platform implementation calls to impl_icu::DateTimeFormat.
- Edit the order of Intl.DateTimeFormat options list in Intl.cpp so that they match the spec's option read ordering. A few test262 intl DateTimeFormat tests check for option read order and would fail if the options in this list are not ordered in the spec's option read ordering.
- Add reading and validating formatMatcher option in PlatformIntlApple.mm.
- Add new test files: date-time-format-additional.js, date-time-format-hour-cycle.js, date-time-format-format-range.js. They tests Intl.DateTimeFormat features that are not supported on Apple, and thus labeled with "UNSUPPORTED: apple". The test runner will skip these when running on Apple platform.
- Rename date-time-format-apple.js to date-time-format.js, which have common test cases for both Apple and Linux / ICU4C-backed Intl.DateTimeFormat implementation. Edit a few test cases and expectations to account for differences in locale data and time zone id support (only use IANA time zone ids as they are the only ones that the spec allows). Add more negative test cases for invalid option values.
- Enable running more test262 Intl test cases by editing testsuite.py to no longer skip tests that include testintl.js and tests that use 'const' declaration.
- Update corresponding skip lists so to skip tests that are failing due to unsupported features for Hermes in general, for Linux only, or for Apple only.
Test Plan
Builds succeed on Ubuntu 20.04, 24.04, and Mac. Run the following JS tests on Ubuntu 20.04, 24.04, Mac, and all pass:
$ ./build/bin/hermes ./hermes/test/hermes/intl/intl.js
$ ./build/bin/hermes ./hermes/test/hermes/intl/get-canonical-locales.js | ./build/bin/FileCheck --match-full-lines ./hermes/test/hermes/intl/get-canonical-locales.js
$ ./build/bin/hermes ./hermes/test/hermes/intl/collator.js | ./build/bin/FileCheck --match-full-lines ./hermes/test/hermes/intl/collator.js
$ LC_ALL=fr_FR _HERMES_TEST_LOCALE=fr_FR ./build/bin/hermes ./hermes/test/hermes/intl/collator-resolved-options.js
$ TZ=GMT ./build/bin/hermes -O -target=HBC ./hermes/test/hermes/intl/date-time-format.js | ./build/bin/FileCheck --match-full-lines ./hermes/test/hermes/intl/date-time-format.js
$ TZ=US/Pacific ./build/bin/hermes -O -target=HBC hermes/test/hermes/intl/default-tz.js | ./build/bin/FileCheck --match-full-lines ./hermes/test/hermes/intl/default-tz.js
Run the following JS tests on Ubuntu 20.04, 24.04, and all pass:
$ ./build/bin/hermes hermes/test/hermes/intl/collator-resolved-options-validate-co-extension.js
$ TZ=GMT ./build/bin/hermes hermes/test/hermes/intl/date-time-format-additional.js | ./build/bin/FileCheck --match-full-lines ./hermes/test/hermes/intl/date-time-format-additional.js
$ TZ=GMT ./build/bin/hermes hermes/test/hermes/intl/date-time-format-hour-cycle.js
$ TZ=GMT ./build/bin/hermes hermes/test/hermes/intl/date-time-format-format-range.js
Run valgrind with above test runs and no memory leak detected.
Run Test262 test suite (pinned at commit hash 62626e083bd506124aac6c799464d76c2c42851b that the github workflow uses for test stablity). All non-skipped test cases pass.
$ ./hermes/utils/testsuite/run_testsuite.py -b ./build/bin --test-intl -a ~/Downloads/test262-62626e0/test262/test/
Hi @neildhar and @tmikov,
This pull request refactors and completes the ICU4C-based Intl.DateTimeFormat implementation. Support for formatToParts, formatRange, formatRangeToParts, as well as offset time zone string inputs, e.g. +0200, -07:00, etc, is added.
Please have a look. Thank you!
Thanks for submitting this PR! Before we start reviewing it, could you make the changes to the static_h branch instead? All new features/improvements should happen on that branch now. Thanks!
Hi @lavenzg,
Thank you for letting me know about static_h branch. I see that Hermes team is working on getting React Native main branch and releases to switch over to the static_h branch: https://github.com/facebook/react-native/pull/48531.
Our team tracks RN main branch and releases as upstreams and Hermes is part of that. So we get upstream Hermes changes from Hermes main branch, until RN main branch and releases switch over to Hermes static_h branch. We would thus like to see this PR to be included in both Hermes main branch and static_h branch as we don't yet know when RN will switch over.
What do you think about keeping this PR targeting main branch, and have it reviewed, comments addressed, and merged first, then subseqeuntly I'll work on a new PR for the static_h branch?
thanks,
Robert
P.S. I took a look at the relevant code files on static_h branch, and bringing the changes in this PR to static_h should be straightforward as the relevant code is the pretty much the same between static_h and main branch, except for some details to be worked out in the automated testing configuration because that setup has some differences between the two branches.
Hi @lavenzg,
Just want to see if you had a chance to start reviewing this yet.
Regards,
Robert
Hi @lavenzg, @neildhar, @tmikov,
Friendly reminder to review this PR.
Regards,
Robert
Hi @lavenzg, @neildhar, @tmikov,
Friendly reminder to review this PR.
Regards,
Robert
Hi Robert, I'm reviewing it. Will get back to you soon.
Hi @lavenzg,
Hope you are doing well. Any update on the review for this PR?
Regards,
Robert
Hi @lavenzg, @neildhar, @tmikov,
Hope things are going fine. Please help review this PR as it's been opened for quite some time.
Regards,
Robert
@lavenzg, thanks for the review and feedback. I plan to address them next week.
@lavenzg, please have a look at my replies to your feedback and the new revision I posted.
I'm going to be on vacation between June 7 - 29. If there are additional feedback while I'm away, I'll address them after I'm back. I will also work on porting this to the static_h branch after my break.
@lavenzg, I'm back in office. Have you had a chance to review rev 2? Thanks!
@lavenzg, I'm back in office. Have you had a chance to review rev 2? Thanks!
LGTM. I will import it to our internal repository and run some extra tests. If no failure we will merge them.
Thanks for your work on this (and your patience as my response sometimes are slow)!
@lavenzg has imported this pull request. If you are a Meta employee, you can view this in D77691166.
@lavenzg has imported this pull request. If you are a Meta employee, you can view this in D77691166.
@lavenzg, are you able to merge this PR? Let me know if you need anything form me.
Hi, unfortunately it seems like we won't be able to merge this PR. I apologize, Meta, for better or worse, has decided that it cannot dedicate the resources to maintain a Intl implementation, and we can't merge something that we can't maintain. This is not an ideal situation, but it is out the hands of the Hermes team.
Instead, we are adding 1st class (and maintained) support for NodeAPI, which is well defined and ABI stable. This make it possible for Intl implementations to exist outside of the Hermes repository, while being guaranteed compatibility with Hermes by NodeAPI.
