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

Wrong Intl.DateTimeFormat output for zh-CN without year part #29596

Closed
duanyao opened this issue Sep 18, 2019 · 9 comments
Closed

Wrong Intl.DateTimeFormat output for zh-CN without year part #29596

duanyao opened this issue Sep 18, 2019 · 9 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@duanyao
Copy link
Contributor

duanyao commented Sep 18, 2019

Test code:

new Intl.DateTimeFormat('zh-CN', { timeZone: 'Asia/Shanghai', month: "2-digit",
  day: "2-digit" }).format(new Date('2019-09-16 GMT+8'))

nodejs v12.10.0 outputs a strange string '09 ├Day: 16┤'.

On nodejs v10.14.2, the result is '09-16', which is correct.

@ex1st
Copy link

ex1st commented Sep 18, 2019

Node v12.10.0

'09 ├Day: 16┤'

Node v12.10.0 with full-icu

'09-16'

@duanyao
Copy link
Contributor Author

duanyao commented Sep 18, 2019

I have verified that both v12.10.0 and v10.14.2 I tested are with small-icu, according to https://nodejs.org/docs/latest-v10.x/api/intl.html

new Intl.DateTimeFormat('es', { month: 'long' }).format(new Date(9e8))
// 'M01'

v10.x is ok for Intl.DateTimeFormat + zh-CN but 12.x is not.

@bnoordhuis bnoordhuis added i18n-api Issues and PRs related to the i18n implementation. v12.x labels Sep 18, 2019
@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 18, 2019

Can't reproduce. Both master and v12.10.0 with small-icu pritn '09/16' for me.

edit: and to be clear: yes, that's a slash, not a dash.

@duanyao
Copy link
Contributor Author

duanyao commented Sep 19, 2019

@bnoordhuis I guess your system locale is en-US or similar (mine is zh-CN). If I run node v12.10.0 with a overwritten locale(LANG=en-US node), I get '09/16' too. This behavior surprises me because I thought Intl.DateTimeFormat did not depend on system locale.

@bnoordhuis
Copy link
Member

With LANG=zh-CN I can reproduce. Seems to be caused by the upgrade to ICU 64.2 in #27361 which means v10.16.0 and newer are also affected.

I don't know if it's actually a bug but @srl295 can probably shed some light on that.

This behavior surprises me because I thought Intl.DateTimeFormat did not depend on system locale.

It does when ICU doesn't know the timezone name you pass to Intl.DateTimeFormat.

Fixing the fallback locale to some known-good value has been discussed in the past but being able to override it is sometimes useful.

@duanyao
Copy link
Contributor Author

duanyao commented Sep 19, 2019

@bnoordhuis I checked v10.16.3 and it doesn't affected (https://nodejs.org/dist/v10.16.3/node-v10.16.3-linux-x64.tar.xz ).

Omitting the timeZone option doesn't change those results for me:

new Intl.DateTimeFormat('zh-CN', { month: "2-digit", day: "2-digit" }).format(new Date('2019-09-16 GMT+8'))

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 19, 2019

Hm, right. I reverted the ICU upgrade in my local v10.x branch and it indeed makes no difference.

I suppose that means a change in V8 is responsible (or the interaction between ICU and V8) but the diff between V8 6.8 and 7.6 is so big that it's hopeless to isolate the change.

There are plans to bundle full-icu with Node.js (#19214) so this issue will probably become moot in the not too distant future.

Omitting the timeZone option doesn't change those results for me:

Yes, I should have said "timezone or locale." (I did refer to the locale in the next sentence but not the first one.)

@duanyao
Copy link
Contributor Author

duanyao commented Sep 19, 2019

Sounds good to me. For now my workaround is adding a year: "numeric" option and picking month and date parts from the result. Feel free to close this issue.

@bnoordhuis
Copy link
Member

Okay, I'll go ahead and do that.

For everyone coming here through search engines: installing the full-icu package should fix this in a forward compatible manner.

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

3 participants