Implement getCanonicalLocales in non apple/android environments

[manuelpuyol]

Related to #1350 and #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]

@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

hermes/CMakeLists.txt

Lines 515 to 519 in d62b96e

	# Attempt to use system ICU first, if none specified.
	# Don't need ICU on Apple, Emscripten, and Android.
	if (APPLE OR EMSCRIPTEN OR HERMES_IS_ANDROID)
	set(ICU_FOUND 1)
	endif()

[neildhar]

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]

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

[neildhar]

LGTM apart from the minor build change, looking at your other diff, it looks like getCanonicalLocales might belong in PlatformIntlShared.cpp, but I'll leave it up to you.

Regarding the test failure, I suspect something just broke in RN mainline, it will probably clear up once it is fixed.

[neildhar]

We need to make sure we link against ICU

Suggested change

	add_hermes_library(hermesPlatformIntl STATIC PlatformIntlICU.cpp
	LINK_LIBS
	hermesBCP47Parser
	hermesPublic
	)
	add_hermes_library(hermesPlatformIntl STATIC PlatformIntlICU.cpp
	LINK_LIBS
	hermesBCP47Parser
	hermesPublic
	)
	hermes_link_icu(hermesPlatformIntl)

[manuelpuyol]

we aren't using anything from ICU in this implementation, should we still link it?

[neildhar]

I suppose not in this particular PR. If you'd prefer to do it in the next PR once ICU is being used, we can just do that.

[manuelpuyol]

cool, I'm linking it in the DateTimeFormat PR so we should be good :)

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

@neildhar merged this pull request in ef5fa1a.