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

Number.toLocaleString output is inconsistent between minor versions. #21053

Closed
ken210 opened this issue May 31, 2018 · 12 comments
Closed

Number.toLocaleString output is inconsistent between minor versions. #21053

ken210 opened this issue May 31, 2018 · 12 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@ken210
Copy link

ken210 commented May 31, 2018

  • Version: 10.3.0
  • Platform: Darwin Kernel Version 17.5.0
  • Subsystem: Not sure.

Output of Number.toLocaleString is inconsistent since version v10.3.0
Versions prior to v10.3.0 have consistent output. Tested with v8.11.2, v9.0.0, v10.0.0, v10.2.1

$ node -v
> v10.2.1

$ node -e "console.log((1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'}))"
> US$ 1,999.99

$ node -v
> v10.3.0

$ node -e "console.log((1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'}))"
> $1,999.99
@tniessen
Copy link
Member

Failed to reproduce on Ubuntu 16.04:

$ nvm use 10
Now using node v10.3.0 (npm v6.1.0)
$ node 
> process.versions
{ http_parser: '2.8.0',
  node: '10.3.0',
  v8: '6.6.346.32-node.9',
  uv: '1.20.3',
  zlib: '1.2.11',
  ares: '1.14.0',
  modules: '64',
  nghttp2: '1.29.0',
  napi: '3',
  openssl: '1.1.0h',
  icu: '61.1',
  unicode: '10.0',
  cldr: '33.0',
  tz: '2018c' }
> (1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'})
'US$ 1,999.99'

@silverwind
Copy link
Contributor

Please show process.config.variables and process.versions on both versions.

@silverwind silverwind added the i18n-api Issues and PRs related to the i18n implementation. label May 31, 2018
@ken210
Copy link
Author

ken210 commented May 31, 2018

v10.2.1

> process.versions
{ http_parser: '2.8.0',
  node: '10.2.1',
  v8: '6.6.346.32-node.8',
  uv: '1.20.3',
  zlib: '1.2.11',
  ares: '1.14.0',
  modules: '64',
  nghttp2: '1.29.0',
  napi: '3',
  openssl: '1.1.0h',
  icu: '61.1',
  unicode: '10.0',
  cldr: '33.0',
  tz: '2018c' }
> process.config.variables
{ asan: 0,
  build_v8_with_gn: false,
  coverage: false,
  debug_http2: false,
  debug_nghttp2: false,
  force_dynamic_crt: 0,
  host_arch: 'x64',
  icu_data_in: '../../deps/icu-small/source/data/in/icudt61l.dat',
  icu_endianness: 'l',
  icu_gyp_path: 'tools/icu/icu-generic.gyp',
  icu_locales: 'en,root',
  icu_path: 'deps/icu-small',
  icu_small: true,
  icu_ver_major: '61',
  llvm_version: '0',
  node_byteorder: 'little',
  node_debug_lib: false,
  node_enable_d8: false,
  node_enable_v8_vtunejit: false,
  node_install_npm: true,
  node_module_version: 64,
  node_no_browser_globals: false,
  node_prefix: '/',
  node_release_urlbase: 'https://nodejs.org/download/release/',
  node_shared: false,
  node_shared_cares: false,
  node_shared_http_parser: false,
  node_shared_libuv: false,
  node_shared_nghttp2: false,
  node_shared_openssl: false,
  node_shared_zlib: false,
  node_tag: '',
  node_target_type: 'executable',
  node_use_bundled_v8: true,
  node_use_dtrace: true,
  node_use_etw: false,
  node_use_openssl: true,
  node_use_perfctr: false,
  node_use_v8_platform: true,
  node_without_node_options: false,
  openssl_fips: '',
  openssl_no_asm: 0,
  shlib_suffix: '64.dylib',
  target_arch: 'x64',
  v8_enable_gdbjit: 0,
  v8_enable_i18n_support: 1,
  v8_enable_inspector: 1,
  v8_no_strict_aliasing: 1,
  v8_optimized_debug: 0,
  v8_promise_internal_field_count: 1,
  v8_random_seed: 0,
  v8_trace_maps: 0,
  v8_typed_array_max_size_in_heap: 0,
  v8_use_snapshot: true,
  want_separate_host_toolset: 0,
  xcode_version: '7.0' }
> (1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'})
'US$ 1,999.99'

v10.3.0

> process.versions
{ http_parser: '2.8.0',
  node: '10.3.0',
  v8: '6.6.346.32-node.9',
  uv: '1.20.3',
  zlib: '1.2.11',
  ares: '1.14.0',
  modules: '64',
  nghttp2: '1.29.0',
  napi: '3',
  openssl: '1.1.0h',
  icu: '61.1',
  unicode: '10.0',
  cldr: '33.0',
  tz: '2018c' }
> process.config.variables
{ asan: 0,
  build_v8_with_gn: false,
  coverage: false,
  debug_http2: false,
  debug_nghttp2: false,
  force_dynamic_crt: 0,
  host_arch: 'x64',
  icu_data_in: '../../deps/icu-small/source/data/in/icudt61l.dat',
  icu_endianness: 'l',
  icu_gyp_path: 'tools/icu/icu-generic.gyp',
  icu_locales: 'en,root',
  icu_path: 'deps/icu-small',
  icu_small: true,
  icu_ver_major: '61',
  llvm_version: '0',
  node_byteorder: 'little',
  node_debug_lib: false,
  node_enable_d8: false,
  node_enable_v8_vtunejit: false,
  node_install_npm: true,
  node_module_version: 64,
  node_no_browser_globals: false,
  node_prefix: '/',
  node_release_urlbase: 'https://nodejs.org/download/release/',
  node_shared: false,
  node_shared_cares: false,
  node_shared_http_parser: false,
  node_shared_libuv: false,
  node_shared_nghttp2: false,
  node_shared_openssl: false,
  node_shared_zlib: false,
  node_tag: '',
  node_target_type: 'executable',
  node_use_bundled_v8: true,
  node_use_dtrace: true,
  node_use_etw: false,
  node_use_openssl: true,
  node_use_perfctr: false,
  node_use_v8_platform: true,
  node_without_node_options: false,
  openssl_fips: '',
  openssl_no_asm: 0,
  shlib_suffix: '64.dylib',
  target_arch: 'x64',
  v8_enable_gdbjit: 0,
  v8_enable_i18n_support: 1,
  v8_enable_inspector: 1,
  v8_no_strict_aliasing: 1,
  v8_optimized_debug: 0,
  v8_promise_internal_field_count: 1,
  v8_random_seed: 0,
  v8_trace_maps: 0,
  v8_typed_array_max_size_in_heap: 0,
  v8_use_snapshot: true,
  want_separate_host_toolset: 0,
  xcode_version: '7.0' }
> (1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'})
'$1,999.99'

@silverwind
Copy link
Contributor

Interesting, same ICU version and build but different output. Any idea @nodejs/intl?

@vsemozhetbyt
Copy link
Contributor

Cannot reproduce with Windows builds:
https://nodejs.org/download/release/v10.2.1/win-x64/
https://nodejs.org/download/release/v10.3.0/win-x64/

> node.10.2.1.exe -e "console.log((1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'}))"
US$ 1,999.99

> node.10.3.0.exe -e "console.log((1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'}))"
US$ 1,999.99

@devsnek
Copy link
Member

devsnek commented May 31, 2018

can repro on mac

> n 10.2.1
> node -pe "(1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'})"
US$ 1,999.99

> n 10.3.0
> node -pe "(1999.99).toLocaleString('us', {style: 'currency', currency: 'USD'})"
$1,999.99

@devsnek
Copy link
Member

devsnek commented May 31, 2018

i would guess this is related to #20826

/cc @TimothyGu

@TimothyGu
Copy link
Member

Yes… this was intended to be a fix of a bug that presented itself somewhere between v7.9.0 and v8.4.0 (#15223). I personally don’t believe the bug fix was a breaking change that warrants semver-major, but I do realize that it has the potential to break some test suites, so apologies about that.

I don’t think there is much we can do right now though. After all a revert would just make the problem worse (another such change in the v10.x branch).

@silverwind
Copy link
Contributor

Closing based on #21053 (comment).

@srl295
Copy link
Member

srl295 commented Jun 1, 2018

it has the potential to break some test suites

test suites should not expect specific 'golden' results for locale data.

@msegers
Copy link

msegers commented Feb 6, 2019

@srl295 Maybe, but you'd expect the same result on the same locale between node versions, right? We noticed the issue upgrading to node 10+ so we ended up removing the usage of locale, unfortunately.

@Whobeu
Copy link

Whobeu commented Mar 12, 2019

@srl295 Maybe, but you'd expect the same result on the same locale between node versions, right? We noticed the issue upgrading to node 10+ so we ended up removing the usage of locale, unfortunately.

I just started testing Node 10 for planned migration from Node 8. Noticed that Date.toLocaleString("iso") now behaves differently. Under Node 8 I would get "2019-03-12 12:34:56" but under Node 10+ I am getting "03/12/2019 12:34:56 PM". Funny thing is I am trying to find the old reference to "iso" as a valid locale and I am having no luck. I suspect it may have been expunged. However, toLocaleString() does not throw an error with it like it does for other invalid locales.

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.
Projects
None yet
Development

No branches or pull requests

9 participants