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.

Sign up for GitHub

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

Jump to bottom

Intl.NumberFormat resolvedOptions style is wrong #39050

Closed
snowystinger opened this issue Jun 16, 2021 · 3 comments
Closed

Intl.NumberFormat resolvedOptions style is wrong #39050

snowystinger opened this issue Jun 16, 2021 · 3 comments
Labels
i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.

Comments

@snowystinger
Copy link

  • Version: v14.17.0
  • Platform: Darwin *** 20.4.0 Darwin Kernel Version 20.4.0: Thu Apr 22 21:46:47 PDT 2021; root:xnu-7195.101.2~1/RELEASE_X86_64 x86_64

What steps will reproduce the bug?

(new Intl.NumberFormat('en-US', {style: 'percent'})).resolvedOptions() yields

{
  locale: 'en-US',
  numberingSystem: 'latn',
  style: 'decimal',
  minimumIntegerDigits: 1,
  minimumFractionDigits: 0,
  maximumFractionDigits: 0,
  useGrouping: true,
  notation: 'standard',
  signDisplay: 'auto'
}

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

every time

What is the expected behavior?

the style entry in the resolved options should be 'percent', not 'decimal'

What do you see instead?

Additional information

It was correct in node 12, 14.16.1, and 16.3
Sorry I didn't test more of them but the 14.16 -> 14.17 breaking should be helpful
snowystinger added a commit to adobe/react-spectrum that referenced this issue Jun 16, 2021
@snowystinger
fix format number resolved options
d60dbab 
14.17 introduced a breaking change where resolvedOptions returns 'decimal' instead of 'percent'. I've logged a bug here:
nodejs/node#39050
@snowystinger snowystinger changed the title Intl.NumberFormat resolvedOptions are wrong Intl.NumberFormat resolvedOptions style is wrong Jun 16, 2021
@mscdex
Copy link
Contributor

mscdex commented Jun 16, 2021

Seems to be something that changed from v14.16.1, my guess is #36187. 'percent' is shown on v10.x, v12.x, v14.0.0-v14.16.1, v16.x, and master.

@targos targos added v14.x i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency. labels Jun 16, 2021
@targos
Copy link
Member

targos commented Jun 16, 2021

my guess is #36187.

It was my first guess too, but v14.17.1 also shows the bug and has ICU 69, like v16.x and master.

@targos targos mentioned this issue Jun 16, 2021
Closed
@targos
Copy link
Member

targos commented Jun 16, 2021

This should be fixed by #39051

@targos targos mentioned this issue Jun 16, 2021
Closed
snowystinger added a commit to adobe/react-spectrum that referenced this issue Jun 29, 2021
@snowystinger
Upgrade to node 14 (#2027)
d1645fb 
* Upgrade to node 16

* be more specific with our version and use the new recommended image

* move to 14, once we upgrade parcel, maybe we can go to 16

* fix format number resolved options

14.17 introduced a breaking change where resolvedOptions returns 'decimal' instead of 'percent'. I've logged a bug here:
nodejs/node#39050
@richardlau richardlau mentioned this issue Jul 15, 2021
Closed
targos pushed a commit that referenced this issue Jul 18, 2021
@richardlau @targos
test: add NumberFormat resolvedOptions test
07e83f7 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: #39401
Refs: #39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit to targos/node that referenced this issue Jul 18, 2021
@richardlau @targos
test: add NumberFormat resolvedOptions test
e76736a 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: nodejs#39401
Refs: nodejs#39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
richardlau added a commit to targos/node that referenced this issue Jul 19, 2021
@richardlau
test: add NumberFormat resolvedOptions test
3f81a84 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: nodejs#39401
Refs: nodejs#39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit to targos/node that referenced this issue Jul 20, 2021
@richardlau @targos
test: add NumberFormat resolvedOptions test
ea1f70e 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: nodejs#39401
Refs: nodejs#39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
richardlau pushed a commit that referenced this issue Jul 20, 2021
@targos @richardlau
deps: V8: cherry-pick b0a7f5691113
10dddd7 
Original commit message:

    Update to ICU68-1

    ICU68-1 change the output skeleton format. So we need to change
    resolvedOptions code for 68 migration.

    Chromium roll
    https://chromium-review.googlesource.com/c/chromium/src/+/2474093

    Bug: v8:10945
    Change-Id: I3b2c7fbe8abb22df8fa51287c498ca3245b8c55b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477431
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70972}

Refs: v8/v8@b0a7f56

PR-URL: #39051
Fixes: #39050
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau added a commit that referenced this issue Jul 20, 2021
@richardlau
test: add NumberFormat resolvedOptions test
82dcdb9 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: #39401
Refs: #39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Backport-PR-URL: #39051
@targos targos closed this as completed Jul 20, 2021
richardlau pushed a commit that referenced this issue Jul 20, 2021
@targos @richardlau
deps: V8: cherry-pick b0a7f5691113
9b69069 
Original commit message:

    Update to ICU68-1

    ICU68-1 change the output skeleton format. So we need to change
    resolvedOptions code for 68 migration.

    Chromium roll
    https://chromium-review.googlesource.com/c/chromium/src/+/2474093

    Bug: v8:10945
    Change-Id: I3b2c7fbe8abb22df8fa51287c498ca3245b8c55b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477431
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70972}

Refs: v8/v8@b0a7f56

PR-URL: #39051
Fixes: #39050
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau added a commit that referenced this issue Jul 20, 2021
@richardlau
test: add NumberFormat resolvedOptions test
45c2ea3 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: #39401
Refs: #39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Backport-PR-URL: #39051
targos pushed a commit that referenced this issue Jul 20, 2021
@richardlau @targos
test: add NumberFormat resolvedOptions test
a2df577 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: #39401
Refs: #39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this issue Jul 29, 2021
@richardlau @BethGriggs
test: add NumberFormat resolvedOptions test
04355af 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: #39401
Refs: #39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
@targos @foxxyz
deps: V8: cherry-pick b0a7f5691113
72a9eab 
Original commit message:

    Update to ICU68-1

    ICU68-1 change the output skeleton format. So we need to change
    resolvedOptions code for 68 migration.

    Chromium roll
    https://chromium-review.googlesource.com/c/chromium/src/+/2474093

    Bug: v8:10945
    Change-Id: I3b2c7fbe8abb22df8fa51287c498ca3245b8c55b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2477431
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70972}

Refs: v8/v8@b0a7f56

PR-URL: nodejs#39051
Fixes: nodejs#39050
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
@richardlau @foxxyz
test: add NumberFormat resolvedOptions test
c64894e 
Add a regression test for NumberFormat resolvedOptions.

PR-URL: nodejs#39401
Refs: nodejs#39050
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Backport-PR-URL: nodejs#39051
@snowystinger snowystinger mentioned this issue Dec 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mscdex @snowystinger @targos