Skip to content

Conversation

@satkh
Copy link

@satkh satkh commented Jun 19, 2025

Summary

synchronize Internationalization code on Hermes-windows with Hermes-Meta.
Have commented out the few files in [lib/Platform/Intl/CMakeLists.txt], failing due to Intl not enabled in Hermes, which will be fixed in future Prs.

Test Plan

 Ran the Unit tests, its successful.
Microsoft Reviewers: Open in CodeFlow

@satkh satkh marked this pull request as ready for review June 24, 2025 05:40
@satkh satkh requested a review from a team as a code owner June 24, 2025 05:40
@satkh satkh requested review from vineethkuttan and vmoroz June 24, 2025 05:40
// #include "unicode/uniset.h"
// #include "unicode/unorm2.h"
// #include "unicode/ustring.h"
#include "unicode/ucnv.h"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how it works.
I see that Windows ICU does not have these headers.
https://learn.microsoft.com/en-us/windows/win32/intl/international-components-for-unicode--icu-

Do we require to have additional ICU support library dependencies to be shipped along with hermes-windows?

Choose a reason for hiding this comment

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

Windows SDK has icu.h, icucommon.h and icui18n.h which should include all C APIs in icu which is supported by Windows ..

Including these actual ICU headers are risky, as we may end up referencing functions which are not actually available on ICU DLLs shipped on Windows ..

}

/// https://402.ecma-international.org/8.0/#sec-case-sensitivity-and-case-mapping
std::u16string toASCIIUppercase(std::u16string_view tz) {
Copy link
Member

Choose a reason for hiding this comment

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

I would expect that there is a standard function for it already.


add_hermes_unittest(BCP47ParserTests BCP47ParserTest.cpp)
target_link_libraries(BCP47ParserTests hermesBCP47Parser)
add_hermes_unittest(BCP47ParserTests BCP47ParserTest.cpp)
Copy link
Member

Choose a reason for hiding this comment

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

nit: this file should probably be just reverted.

@vmoroz
Copy link
Member

vmoroz commented Jun 30, 2025

@satkh , we have discussed this issue with @mganandraj offline and my basic understanding of the issue is as the following:

  • Two ICU libraries for Windows:

  • JavaScript Intl support based on ICU support in Hermes

    • Hermes-windows code has a partial implementation that uses one of the latest flavors of the Microsoft Windows ICU APIs.
    • Hermes code has an implementation that relies on the Unicode org ICU.
    • We also have teams and scenarios where developers do not want hermes.dll dependency on an ICU DLL. (E.g. hermes.dll currently crashes on Windows Server 2016 because it does not have the Windows ICU DLLs)
  • The challenges that we have:

    • We need to merge the JS Intl support between the upstream Hermes repo and hermes-windows repo. We can send PRs to the upstream repo when we get a good solution.
    • Ideally, the hermes.dll dependency on specific ICU DLL must be configurable. It must not have a hard dependency on any specific ICU DLL. It must be up to specific use scenario where the developer configures the Hermes Runtime instance to use one or another ICU library.
    • One way to address this scenario is to dynamically load the required ICU library by using the LoadLibrary API.
    • It may be difficult to implement such dynamic system for the actual ICU DLLs because we do not control them and we do not want to change their header files or .lib files that they offer. Instead, we can define our own small API required for the JS Intl support and have one small DLL in our Nuget per each supported variant of the ICU DLL. We dynamically load our ICU wrapper DLLs while they are explicitly linked against the targeted ICU DLLs.
    • If the JS Intl support will be in separate DLLs, then to simplify merges between hermes and hermes-windows we can move all MS Windows specific implementation to a separate folder. This is for the case when we cannot merges the hermes-windows and hermes implementations.

@satkh
Copy link
Author

satkh commented Jul 13, 2025

Converting to draft as Intl is not priority now, will re-open when we resume work on Intl .

@satkh satkh marked this pull request as draft July 13, 2025 15:30
@satkh
Copy link
Author

satkh commented Jul 15, 2025

Closing this PR, will be picked up when we resume work on Intl.

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.

4 participants