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

ICU 68.1 upgrade changes currency formatting output in Node 14.17 #38897

Closed
iansu opened this issue Jun 1, 2021 · 10 comments
Closed

ICU 68.1 upgrade changes currency formatting output in Node 14.17 #38897

iansu opened this issue Jun 1, 2021 · 10 comments
Assignees
Labels
i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency.

Comments

@iansu
Copy link
Contributor

iansu commented Jun 1, 2021

  • Version: 14.17
  • Platform: Darwin
  • Subsystem: ?

What steps will reproduce the bug?

Running this code in Node 14.16 and 14.17 produces different results:

console.log(123.45.toLocaleString('fr-CA', { style: 'currency', currency: 'CAD', currencyDisplay: 'symbol' }));

In Node 14.16 the output is 123,45 $
In Node 14.17 the output is 123,45 $ CA

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

There should be no change in output

What do you see instead?

Different output between minor Node versions (14.16 and 14.17)

Additional information

I'm not certain if this is an issue with Node or the ICU data library. All I know is this output has changed in a minor version release which is unexpected to me and maybe seems like a breaking change. This caused my tests to fail and likely would have caused a production issue if my tests hand't caught it.

The ICU data was upgraded to version 68.1 in Node 14.17 here: #36187

@Ayase-252 Ayase-252 added the icu Issues and PRs related to the ICU dependency. label Jun 2, 2021
@targos
Copy link
Member

targos commented Jun 2, 2021

Updates of CLDR data probably always result in the change of the output in some cases. I don't think it was ever discussed if these would qualify as semver-major. They can be seen as bug fixes too.

FWIW, you can get the previous behavior consistently with:

console.log(123.45.toLocaleString('fr-CA', { style: 'currency', currency: 'CAD', currencyDisplay: 'narrowSymbol' }));

@targos
Copy link
Member

targos commented Jun 2, 2021

/cc @nodejs/i18n-api

@srl295
Copy link
Member

srl295 commented Jun 2, 2021

Yes, I would be surprised if there wasn't a change in output when new CLDR data comes in . You can check the CLDR version with node -p 'process.versions.cldr

@iansu :

This caused my tests to fail and likely would have caused a production issue if my tests hand't caught it.

I would like to hear more about the use case here. Specifically, how this would produce a production issue.. It is not recommended to have tests which explicitly verify formatted output, it's for humans to see.

For example, for network transit between software, disk storage, etc. a binary or locale-neutral format ought to be used. I'd be happy to help comment on any design approach if I can help.

@srl295 srl295 self-assigned this Jun 2, 2021
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Jun 2, 2021
@srl295
Copy link
Member

srl295 commented Jun 2, 2021

Example of other tickets that are locale data changes: #34484 #31374

@srl295
Copy link
Member

srl295 commented Jun 2, 2021

@obensource a good example of a recent CLDR change

@nodejs/i18n-api what is the right label for "locale data changed due to CLDR"? wontfix?

@iansu
Copy link
Contributor Author

iansu commented Jun 2, 2021

@targos Thank you for suggesting that fix. That does seem to resolve the issue.

I can see how this is expected to happen but it would be great if this was somehow more clear. This was a surprising change to me and it also took quite a while for me to figure out what was going on, where to report it, if I should report it at all, etc. I don't have any concrete solutions for this but I'm active in this project and I was confused so this would probably be confusing to others as well.

I would like to hear more about the use case here. Specifically, how this would produce a production issue.. It is not recommended to have tests which explicitly verify formatted output, it's for humans to see.

I don't mean production issue as in a crash or an outage. I'm using this to generate financial documents and in some cases there are rules around how things need to be displayed. This would have changed those documents and, given the additional characters in the new format, could have caused other formatting issues.

@srl295
Copy link
Member

srl295 commented Jun 2, 2021

@targos Thank you for suggesting that fix. That does seem to resolve the issue.

That is specifically requesting a different display, but is definitely not guaranteed to be identical.

I can see how this is expected to happen but it would be great if this was somehow more clear.

I certainly would like to improve anywhere that we can.

This was a surprising change to me and it also took quite a while for me to figure out what was going on, where to report it, if I should report it at all, etc.

It's certainly fine to report it here. ( @obensource this is where we need to make the linkage node -> v8 -> ICU -> CLDR more clear. )

I would like to hear more about the use case here. Specifically, how this would produce a production issue.. It is not recommended to have tests which explicitly verify formatted output, it's for humans to see.

I don't mean production issue as in a crash or an outage. I'm using this to generate financial documents and in some cases there are rules around how things need to be displayed. This would have changed those documents and, given the additional characters in the new format, could have caused other formatting issues.

If there are rules about how things are to be displayed, locale sensitive output may not be the right choice for you, because you would need to implement those rules. If the rules say, "ASCII digits, followed by dollarsign" …  etc.

The purpose of the locale strings has always been to attempt to output something that will be readable to people with their own language and cultural preferences. Think of a general user viewing some financial information. As I mentioned, if your documents have a specific requirement, they fall somewhat outside of this.

Ideally you would use an Intl.NumberFormat but specify your own data rather than loading fr-CA data.

( @sffc do you know of any ecma402 proposals that allow specifying one's own pattern and symbol information, as with a locale-less ICU formatter?)

@sffc
Copy link

sffc commented Jun 4, 2021

I'm using this to generate financial documents and in some cases there are rules around how things need to be displayed. This would have changed those documents and, given the additional characters in the new format, could have caused other formatting issues.

The default currency symbol is the CLDR short symbol, which is intended as a symbol which is unambiguously understood in that locale. You can see that this symbol was changed in CLDR 38 to add the "CA" to the end:

https://unicode-org.github.io/cldr-staging/charts/38/delta/fr.html#Northern%20America:%20Canada

The suggestion above to use currencyDisplay: "narrowSymbol" is useful if there is additional context on the page to disambiguate the currency.

( @sffc do you know of any ecma402 proposals that allow specifying one's own pattern and symbol information, as with a locale-less ICU formatter?)

Not at this time. If someone requires a very specific output format, then Intl is probably not what they are looking for. The string outputs from Intl are locale-dependent and could change over time.

However, something that could be in scope for ECMA-402 would be currencyDisplay: "hidden" in order to print the digits in a currency style but leave it up to the context to provide the currency information.

@srl295
Copy link
Member

srl295 commented Sep 24, 2021

@iansu I think the remaining issues in this issue are in two categories:

This was a surprising change to me

it also took quite a while for me to figure out what was going on, where to report it, if I should report it at all, etc

Any ideas? Where did/would you look?

StuAA78 added a commit to DEFRA/sroc-charging-module-api that referenced this issue Feb 18, 2022
While working on a feature, we found that our unit tests were failing during CI but not locally, and the same test was passing yesterday in CI. The failing test was being caused by `toLocaleString()` now returning `Sept` instead of `Sep` for September, which breaks date formatting for transaction and customer files.

On investigation we found that the version of Node being used by Docker was previously 16.13.2 but was now 16.4.0. It appears that the underlying internationalisation data that node uses has changed between these versions. As stated in a [previous related error](nodejs/node#38897):

> The purpose of the locale strings has always been to attempt to output something that will be readable to people with their own language and cultural preferences. Think of a general user viewing some financial information. As I mentioned, if your documents have a specific requirement, they fall somewhat outside of this.

Since the output of `toLocaleString()` cannot be relied on to be consistent, we ditch it completely and instead construct the date string ourselves.
@srl295
Copy link
Member

srl295 commented Apr 4, 2023

Closing as not an issue…

@srl295 srl295 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency.
Projects
None yet
Development

No branches or pull requests

5 participants