Skip to content

Regression in Intl.NumberFormat for the nb-NO locale #39056

New issue

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

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

Already on GitHub? # to your account

Closed
pearofducks opened this issue Jun 16, 2021 · 16 comments
Closed

Regression in Intl.NumberFormat for the nb-NO locale #39056

pearofducks opened this issue Jun 16, 2021 · 16 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@pearofducks
Copy link

  • Version: 16.3.0
  • Platform: MacOS 11.2.3 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64
  • Subsystem: Intl.NumberFormat

What steps will reproduce the bug?

(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)

Run the above command in Node 16.

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

Every time.

What is the expected behavior?

Output of: '200 000'

(Reproduces in Node 15.14.0)

What do you see instead?

Output of: '200,000'

Additional information

@aduh95 aduh95 added the i18n-api Issues and PRs related to the i18n implementation. label Jun 16, 2021
@Isokaeder
Copy link

Isokaeder commented Jun 16, 2021

Same for me, just to add another bit of detail:

new Intl.NumberFormat('nb-NO', {
	style: 'currency',
	currency: 'NOK',
}).format(200_000)

Expected:

"kr 200 000,00"
(works in Node 14.17.0)

Actual

"NOK 200,000.00"

@richardlau
Copy link
Member

cc @nodejs/i18n-api

@ryzokuken
Copy link
Contributor

The result is correct. Engines return the same value:

$ eshost -e "(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)"
#### JavaScriptCore
200 000

#### SpiderMonkey
200 000

#### V8
200 000

You can also check that the same value is mentioned in the CLDR data: https://unicode-org.github.io/cldr-staging/charts/latest/verify/numbers/nb.html

If the value has changed recently, that must be because of an update in the CLDR data. If you feel that this is incorrect, please file an issue for CLDR.

@Linkgoron
Copy link
Member

Linkgoron commented Jun 16, 2021

The result is correct. Engines return the same value:

$ eshost -e "(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)"
#### JavaScriptCore
200 000

#### SpiderMonkey
200 000

#### V8
200 000

You can also check that the same value is mentioned in the CLDR data: https://unicode-org.github.io/cldr-staging/charts/latest/verify/numbers/nb.html

If the value has changed recently, that must be because of an update in the CLDR data. If you feel that this is incorrect, please file an issue for CLDR.

I think you misread the issue, the issue is that there isn't a space and instead there's a comma.

This reproduces for me on Node 16.0. This is probably because of the ICU 69.1 upgrade, which included some major changes to how Norwegian is handled (http://cldr.unicode.org/index/downloads/cldr-39)

@srl295
Copy link
Member

srl295 commented Jun 16, 2021

The result is correct. Engines return the same value:

$ eshost -e "(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)"
#### JavaScriptCore
200 000

#### SpiderMonkey
200 000

#### V8
200 000

You can also check that the same value is mentioned in the CLDR data: https://unicode-org.github.io/cldr-staging/charts/latest/verify/numbers/nb.html
If the value has changed recently, that must be because of an update in the CLDR data. If you feel that this is incorrect, please file an issue for CLDR.

I think you misread the issue, the issue is that there isn't a space and instead there's a comma.

This reproduces for me on Node 16.0. This is probably because of the ICU 69.1 upgrade, which included some major changes to how Norwegian is handled (http://cldr.unicode.org/index/downloads/cldr-39)

yes, and i'm wondering if there was a packaging issue in v8. Investigating…

@srl295
Copy link
Member

srl295 commented Jun 16, 2021

Welcome to Node.js v16.3.0.
Type ".help" for more information.
> (new Intl.NumberFormat('no', { style: 'decimal' })).format(200_000)
'200 000'
> (new Intl.NumberFormat('nb', { style: 'decimal' })).format(200_000)
'200,000'
> (new Intl.NumberFormat('nn', { style: 'decimal' })).format(200_000)
'200 000'
> process.versions.v8
'9.0.257.25-node.16'

fishy

@srl295
Copy link
Member

srl295 commented Jun 16, 2021

> new Intl.DateTimeFormat("nb").resolvedOptions().locale
'en-US'

seems like a v8 problem

@srl295
Copy link
Member

srl295 commented Jun 16, 2021

i'll double check that ICU is working properly.

@Linkgoron
Copy link
Member

Linkgoron commented Jun 16, 2021

Note that this also reproduces with 14.17.1 but not with 14.17, which was also upgraded to ICU 69.1

@ryzokuken
Copy link
Contributor

I think you misread the issue, the issue is that there isn't a space and instead there's a comma.

Yeah, my bad. Let me look deeper.

Thanks @srl295

@ryzokuken
Copy link
Contributor

Updated engine versions and @srl295 wait right, at the very least, this is a V8 problem.

$ eshost -e "(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)"
#### JavaScriptCore
200 000

#### SpiderMonkey
200 000

#### V8
200,000

@srl295
Copy link
Member

srl295 commented Jun 16, 2021

Updated engine versions and @srl295 wait right, at the very least, this is a V8 problem.


$ eshost -e "(new Intl.NumberFormat('nb-NO', { style: 'decimal' })).format(200_000)"

#### JavaScriptCore

200 000



#### SpiderMonkey

200 000



#### V8

200,000

Can you pass it on to v8 or need me to?

@ryzokuken
Copy link
Contributor

@srl295 I'll file an issue on V8

@ryzokuken
Copy link
Contributor

ryzokuken commented Jun 17, 2021

https://bugs.chromium.org/p/v8/issues/detail?id=11897, closing this issue.

@camillobruni
Copy link
Contributor

This has been fixed on V8 a while ago, sadly we're not doing backmerges to older V8 versions.
This will have to happen on the node-side I think (could you maybe double check the icu-lib versions used in these versions?)

v8-8.9.255
200 000
v8-9.0.257
200 000
v8-9.1.269
200 000


v8-9.2.230
200,000
v8-9.3.345
200,000


v8-9.4.146
200 000
v8-9.5.172
200 000
v8-9.6.180
200 000
v8-9.7.106
200 000
v8-9.8.177
200 000
v8-9.9.115
200 000
v8-10.0.139
200 000
v8-10.1.124
200 000
v8-10.2.154
200 000
v8-10.3.174
200 000
v8-10.4.132
200 000

@richardlau
Copy link
Member

I think this is related to #45784. I noted that we did update V8 in Node.js 16 in Node.js 16.9.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

8 participants