lib: set default ICU locale per cli option `--icu-locale` #50538

Uzlopak · 2023-11-03T10:35:08Z

This PR adds a standardized way via the cli option --icu-locale to set the default locale of the ICU used in node process.

How to use:

node --icu-locale=fr-FR

then in the repl you can do

console.log(Intl?.Collator().resolvedOptions().locale) // output is 'fr-FR'

Todo:

get feedback if it is implemented correctly from other maintainers
add tests
add documentation

nodejs-github-bot · 2023-11-03T10:35:13Z

Review requested:

@nodejs/startup

tniessen

I understand the motivation here, as in, providing a portable way of defining the default ICU locale.

However, ICU already does a good job at determining the correct locale based on the environment, and changing ICU's locale may introduce inconsistencies within Node.js applications. It won't affect the locale used by the C and C++ standard libraries, it won't affect other dependencies of Node.js that don't use ICU, it won't correctly propagate to native addons, it won't correctly propagate to child processes, etc.

See also #28099 (comment). cc @bnoordhuis

bnoordhuis · 2023-11-03T16:29:22Z

Another thing: this should use NODE_OPTIONS instead of inventing a new environment variable.

Uzlopak · 2023-11-03T16:46:40Z

I will have a look later regarding implementing it as NODE_OPTIONS.

Uzlopak · 2023-11-03T22:54:09Z

@bnoordhuis

I hope this is to your liking?

doc/api/cli.md

src/node_i18n.cc

test/parallel/test-icu-locale.js

Uzlopak · 2023-11-04T10:55:40Z

@bnoordhuis PTAL

bnoordhuis

Code changes themselves LGTM. I don't know if it's a good change for the reasons Tobias points out.

src/node_i18n.cc

tniessen

Like Ben, I am also not sure if this is a good change.

In any case, if this doesn't properly change the locale for the entire process tree, it should probably be called --icu-locale. See also #50538 (review) and #28099 (comment).

Uzlopak · 2023-11-06T10:23:19Z

@tniessen
Renamed it to --icu-locale

Uzlopak · 2023-11-09T12:43:39Z

Anybody knows how I could get the value of Intl?.Collator().resolvedOptions().locale without a roundtrip to js and back?

bnoordhuis · 2023-11-09T16:54:38Z

It's icu::Locale::toLanguageTag<std::string>()

Uzlopak · 2023-11-15T02:56:31Z

@bnoordhuis

Do you have a code example, please? It doesnt seem trivial to use it.

Uzlopak · 2024-08-14T10:15:38Z

@bnoordhuis
@tniessen

What about now?

codecov · 2024-08-14T11:33:27Z

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (d0f5943) to head (fe4f31b).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/node_i18n.cc	88.88%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #50538   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         648      648           
  Lines      182213   182238   +25     
  Branches    34961    34968    +7     
=======================================
+ Hits       158703   158729   +26     
+ Misses      16794    16784   -10     
- Partials     6716     6725    +9

Files	Coverage Δ
src/node.cc	`74.14% <100.00%> (+0.11%)`	⬆️
src/node_i18n.h	`80.00% <ø> (ø)`
src/node_options.cc	`88.16% <100.00%> (+0.01%)`	⬆️
src/node_options.h	`98.17% <ø> (ø)`
src/node_i18n.cc	`76.36% <88.88%> (+0.47%)`	⬆️

... and 18 files with indirect coverage changes

srl295 · 2024-09-06T16:49:28Z

doc/api/cli.md

+representing the language version as defined in [RFC 5646][] (also known as
+BCP 47).


Suggested change

representing the language version as defined in [RFC 5646][] (also known as

BCP 47).

representing the language version as defined in [IETF BCP 47][].

don't refer to 5646, that's the reason there's a BCP 47.

srl295 · 2024-09-06T16:49:51Z

doc/api/cli.md

 [OSSL_PROVIDER-legacy]: https://www.openssl.org/docs/man3.0/man7/OSSL_PROVIDER-legacy.html
 [Permission Model]: permissions.md#permission-model
 [REPL]: repl.md
+[RFC 5646]: https://www.rfc-editor.org/rfc/rfc5646.txt


Suggested change

[RFC 5646]: https://www.rfc-editor.org/rfc/rfc5646.txt

[BCP 47]: https://www.rfc-editor.org/info/bcp47

src/node_i18n.cc

srl295 · 2024-09-06T16:52:32Z

src/node_i18n.cc

+  const char* newDefaultLocale = uloc_getDefault();
+  int newDefaultLocaleLen = strlen(newDefaultLocale);
+  int32_t locCount = 0;
+  const icu::Locale* supportedLocales =
+      icu::Locale::getAvailableLocales(locCount);
+  for (int32_t i = 0; i < locCount; ++i) {
+    if (strncmp(newDefaultLocale,
+                supportedLocales[i].getName(),
+                newDefaultLocaleLen) == 0) {
+      return;
+    }
+  }


I'm not sure of the purpose of this. Just let the user set the default locale to whatever they want. The user's own system might be set to something not available.

You can set the --icu-locale to invalid values and as long as it is in the right format, it will set it. It will only go to und if it is an invalid locale.This ensures, that the locale is existing and valid.

Tbh. I didnt know how I could get the BestMatcher Class running. So I wrote this logic.

what's the purpose of trying to protect the user from invalid values?

See comment by @bnoordhuis

#50538 (comment)

srl295 · 2024-09-06T16:54:16Z

test/parallel/test-icu-locale.js

+      '-p',
+      'new Date(Date.UTC(2012, 11, 20, 3, 0, 0)).toLocaleString(undefined,  { timeZone: "UTC" })',
+    ]);
+  assert.strictEqual(child.stdout.toString(), '2012. 12. 20. 오전 3:00:00\n');


I really don't like these kinds of tests. At all. You are hard coding linguistic data that is guaranteed to change with every release, and even with local modifications the user makes.

I would recommend NOT testing for unsupported locales (you can't predict which locales are included), and not testing for specific formats. We've discussed this elsewhere.

I took these examples from MDN to ensure it works as expected

You can't expect it. The data changes every single time. See https://www.unicode.org/cldr/charts/latest/delta/index.html and click on any locale.

I think a better test would simply be to verify that the output is DIFFERENT when a different locale is chosen.

right. that is also a good approach.

srl295 · 2024-09-06T16:54:52Z

test/parallel/test-icu-locale.js

+      '-p',
+      'new Intl.ListFormat(undefined, {style: "long", type:"conjunction"}).format(["a", "b", "c"])',
+    ]);
+  assert.strictEqual(child.stdout.toString(), 'a, b und c\n');


perhaps just keep ONE of these tests, to see that setting the locale makes a difference. Knowing that test might need updates. Delete the rest of the tests.

I think two are needed each?! On my machine the locale is de-DE so this test would pass always on my machine, even if the locale flag would not work.

maybe two. you can also set LC_ALL on a unix type env to show the distinction betwen the env and the parameter

$ env LC_ALL=de node -p 'new Date().toLocaleString()' 6.9.2024, 12:14:35 $ env LC_ALL=en node -p 'new Date().toLocaleString()' 9/6/2024, 12:14:39 PM

But then i could not test it on windows?

Uzlopak · 2024-09-06T17:23:06Z

@srl295
I followed you on twitter, if you like to chat about it :)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 3, 2023

Uzlopak mentioned this pull request Nov 3, 2023

lib: add navigator.language and navigator.languages #50303

Merged

Uzlopak changed the title ~~lib: set defautlt locale per cli option --locale~~ lib: set default locale per cli option --locale Nov 3, 2023

Uzlopak changed the title ~~lib: set default locale per cli option --locale~~ lib: set default locale per cli option --locale or LOCALE env var Nov 3, 2023

Uzlopak force-pushed the set-locale-per-cli branch 2 times, most recently from a5b21c6 to ad61226 Compare November 3, 2023 15:21

Uzlopak marked this pull request as ready for review November 3, 2023 15:22

tniessen reviewed Nov 3, 2023

View reviewed changes

Uzlopak force-pushed the set-locale-per-cli branch from ad61226 to 423f20b Compare November 3, 2023 22:53

Uzlopak changed the title ~~lib: set default locale per cli option --locale or LOCALE env var~~ lib: set default locale per cli option --locale Nov 3, 2023

bnoordhuis reviewed Nov 4, 2023

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

src/node_i18n.cc Outdated Show resolved Hide resolved

test/parallel/test-icu-locale.js Outdated Show resolved Hide resolved

Uzlopak force-pushed the set-locale-per-cli branch from 423f20b to 49423d0 Compare November 4, 2023 10:55

bnoordhuis reviewed Nov 4, 2023

View reviewed changes

src/node_i18n.cc Show resolved Hide resolved

tniessen reviewed Nov 6, 2023

View reviewed changes

Uzlopak force-pushed the set-locale-per-cli branch from 49423d0 to 9aea65e Compare November 6, 2023 10:20

Uzlopak changed the title ~~lib: set default locale per cli option --locale~~ lib: set default ICU locale per cli option --icu-locale Nov 6, 2023

Uzlopak force-pushed the set-locale-per-cli branch 2 times, most recently from 70a8143 to 7863b37 Compare August 14, 2024 10:15

lib: set ICU locale per cli using --icu-locale

fe4f31b

Uzlopak force-pushed the set-locale-per-cli branch from 7863b37 to fe4f31b Compare August 14, 2024 10:20

srl295 reviewed Sep 6, 2024

View reviewed changes

		representing the language version as defined in [RFC 5646][] (also known as
		BCP 47).

	[RFC 5646]: https://www.rfc-editor.org/rfc/rfc5646.txt
	[BCP 47]: https://www.rfc-editor.org/info/bcp47

Uh oh!

lib: set default ICU locale per cli option --icu-locale #50538

Are you sure you want to change the base?

lib: set default ICU locale per cli option --icu-locale #50538

Uh oh!

Conversation

Uzlopak commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 3, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak commented Nov 3, 2023

Uh oh!

Uzlopak commented Nov 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uzlopak commented Nov 4, 2023

Uh oh!

bnoordhuis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Nov 6, 2023

Uh oh!

Uzlopak commented Nov 9, 2023

Uh oh!

bnoordhuis commented Nov 9, 2023

Uh oh!

Uzlopak commented Nov 15, 2023

Uh oh!

Uzlopak commented Aug 14, 2024

Uh oh!

codecov bot commented Aug 14, 2024

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Sep 6, 2024

Uh oh!

lib: set default ICU locale per cli option `--icu-locale` #50538

lib: set default ICU locale per cli option `--icu-locale` #50538

Uzlopak commented Nov 3, 2023 •

edited

Loading

bnoordhuis commented Nov 3, 2023 •

edited

Loading

bnoordhuis left a comment •

edited

Loading

Uzlopak Sep 6, 2024 •

edited

Loading