Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decimal and grouping separator for en-ZA does not align with in-country usage #48120

Closed
abster opened this issue May 22, 2023 · 16 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@abster
Copy link

abster commented May 22, 2023

What is the problem this feature will solve?

Currently, the built-in number and currency formatting libraries in Node.js return "," (comma) as decimal separator and " " (whitespace) as grouping separator for formatted numbers and currencies in en-ZA (English South Africa) locale.

console.log(new Intl.NumberFormat('en-ZA', { maximumSignificantDigits: 3 }).format(4.56));
// Expected output: "4.56"
// Actual output: "4,56"

console.log(new Intl.NumberFormat('en-ZA', { style: 'currency', currency: 'SAR' }).format(1234.56));
// Expected output: "SAR 1,234.56"
// Actual output: "SAR 1 234,56"

console.log(new IntlMessageFormat(
  'The price is: {price, number, ::currency/SAR}',
  'en-ZA'
).format({price: 1234.56}));
// Expected output: "The price is: SAR 1,234.56"
// Actual output: "The price is: SAR 1 234,56"

Officially, comma is the decimal separator and space is the grouping separator for numbers in en-ZA (https://www.sadev.co.za/content/how-correctly-format-currency-south-africa), however in common usage, price labels in shops, bank documents and statements use period (.) as decimal separator and comma (,) as grouping separator.

For context, check out following CLDR issues:

Decimal separator - South Africa
Decimal and grouping separator for en_ZA does not align with in-country usage

What is the feature you are proposing to solve the problem?

Upgrade CLDR data in NodeJS. The fix for numbers and currencies in en-ZA locale from CLDR is set to be released as part of CLDR 43.1. This CLDR update will be integrated in an upcoming minor release for ICU4C/ICU4J. Node 20 currently uses ICU 73, while Node 18 (current stable release) uses ICU 72.

  • Node 18 - Upgrade to ICU4C 73 and pick up the next minor release (when it is available) including the fix for numbers and currencies in en-ZA locale in CLDR 43.1.
  • Node 20 - Pick up the next minor release for ICU4C 73 (when it is available) including the fix for numbers and currencies in en-ZA locale in CLDR 43.1.

What alternatives have you considered?

Create overrides in our consumer library. Unfortunately, it is not feasible for us to create overrides for every number and price formatting use case.

@abster abster added the feature request Issues that request new features to be added to Node.js. label May 22, 2023
@aduh95
Copy link
Contributor

aduh95 commented May 22, 2023

@nodejs/i18n

@srl295
Copy link
Member

srl295 commented May 22, 2023

@abster Hi! Good to see you here. Yeah, can pick up the ICU dot release when it's available…

@Antonius-S
Copy link

@abster
Copy link
Author

abster commented Jun 16, 2023

@Antonius-S From my understanding CLDR data can be updated on a specific ICU major release but updating CLDR data on a previous ICU release may not always be compatible, since CLDR releases are tied to specific ICU major releases.

@abster
Copy link
Author

abster commented Jun 16, 2023

Just as a heads-up, ICU 73.2 has been released: https://github.com/unicode-org/icu/releases/tag/release-73-2. Updating node.js to use ICU4C-73.2 should include the fix for en-ZA (addressed in CLDR issue).

@richardlau
Copy link
Member

To set expectations, our automation should pick up the new ICU release over the weekend. There's no reason to rush this any earlier as we won't be able to run the CI on it until the security releases are done next week.

@Antonius-S
Copy link

Antonius-S commented Jun 20, 2023

@abster

@Antonius-S From my understanding CLDR data can be updated on a specific ICU major release but updating CLDR data on a previous ICU release may not always be compatible, since CLDR releases are tied to specific ICU major releases.

According to docs, for external ICU to work, the binary should be built with at most small-icu option so this is just a temporary workaround to solve the issue until new build is ready

@abster
Copy link
Author

abster commented Jul 7, 2023

@richardlau - I see that icu version is updated to 73.2 in the main branch: https://github.com/nodejs/node/blob/main/tools/icu/current_ver.dep

The latest minor release (v18.6.1) for node 18 is however still on 72.1. Do you know when node 18 would be updated to ICU 73.2?

https://github.com/nodejs/node/blob/v18.16.1/tools/icu/current_ver.dep

@richardlau
Copy link
Member

We usually want changes in a current release for at least two weeks before landing in LTS -- I see #48502 only just recently went out in Node.js 20.4.0 so it might be a bit soon for the being prepared 18.17.0 (#48694).

@abster
Copy link
Author

abster commented Jul 26, 2023

@richardlau - The latest minor release (v18.17.0) for node 18 is on ICU 73.1. Do you know when you expect node 18 to be updated to ICU 73.2?

@richardlau
Copy link
Member

Sometime in August according to the current plan in nodejs/Release#737. May be a little clearer after tomorrow's working group meeting where we usually look at the schedules and see who's available to do releases nodejs/Release#886.

@srl295
Copy link
Member

srl295 commented Jul 26, 2023

@abster wrote:

@Antonius-S From my understanding CLDR data can be updated on a specific ICU major release but updating CLDR data on a previous ICU release may not always be compatible, since CLDR releases are tied to specific ICU major releases.

Hi…

Correct, prior ICUs cannot be upgraded to arbitrary CLDR versions. If there's enough interest/available hands to help, a CLDR fix could be back-ported to a prior CLDR maint line, which could then be picked up by the corresponding ICU maint line.

@abster
Copy link
Author

abster commented Sep 1, 2023

(v18.17.1) is on ICU 73.1. Is there a planned release for node 18, which will include update to ICU 73.2? Also, is there a target release date?

@richardlau
Copy link
Member

FYI @ruyadorno perhaps we can consider the ICU update (#48502) for #49220.

@alexmojaki
Copy link

Looks like there was a change recently, and I see the two PRs above are merged. Is this done?

@aduh95
Copy link
Contributor

aduh95 commented Oct 17, 2023

The ICU update has landed in all supported release lines, I’m going to assume this is fixed. Please comment or reopen if the issue still remains.

@aduh95 aduh95 closed this as completed Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
Status: Pending Triage
Development

No branches or pull requests

7 participants
@srl295 @alexmojaki @abster @richardlau @aduh95 @Antonius-S and others