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

Error Date.toLocaleString() in node 19.0.0 #45171

Closed
dmitriym09 opened this issue Oct 25, 2022 · 14 comments · Fixed by #45573
Closed

Error Date.toLocaleString() in node 19.0.0 #45171

dmitriym09 opened this issue Oct 25, 2022 · 14 comments · Fixed by #45573
Labels
confirmed-bug Issues with confirmed bugs. i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.

Comments

@dmitriym09
Copy link

dmitriym09 commented Oct 25, 2022

Version

19.0.0

Platform

Linux 6.0.2-zen1-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Sat, 15 Oct 2022 14:00:51 +0000 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

node

> new Date(new Date().toLocaleString('en-US'))
Invalid Date
> const s = new Date().toLocaleString('en-US')
undefined
> s.at(-3)
' '
> s.at(-3).charCodeAt()
8239
> ' '.charCodeAt()
32
> 

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

Stability

What is the expected behavior?

No response

What do you see instead?

Error Date.toLocaleString(): NARROW NO-BREAK SPACE (U+202f) without last whitespace

Additional information

Thanks for node) It`s cool)

@richardlau
Copy link
Member

NARROW NO-BREAK SPACE sounds like it's from the CLDR 42 change in ICU 72.1
https://icu.unicode.org/download/72

In many formatting patterns, ASCII spaces are replaced with Unicode spaces (e.g., a "thin space").

except Node.js 19.0.0 doesn't ship with ICU 72.1 as that only landed on main today (#45068). Did you build Node.js yourself?

@richardlau richardlau added the i18n-api Issues and PRs related to the i18n implementation. label Oct 25, 2022
@targos
Copy link
Member

targos commented Oct 25, 2022

Another possibility is that you are using a Node.js linked against your system version of ICU, and that version was updated to 72.1.

@dharesign
Copy link
Contributor

Looks like a bug in v8. It's set up to handle unicode whitespace, but instead treats them as keywords. I created https://bugs.chromium.org/p/v8/issues/detail?id=13490

@targos targos added v8 engine Issues and PRs related to the V8 dependency. confirmed-bug Issues with confirmed bugs. labels Nov 15, 2022
@srl295
Copy link
Member

srl295 commented Nov 15, 2022

seems like user error not a bug, don't assume that Localized date format is parseable by Date.

Looking at the v8 issue, I can see where there is a coding bug on the V8 side, and I haven't looked at the spec yet.

But that said, I still disagree that the original example code here should always pass… It makes an invalid assumption about the US date format

Do not use a localized date format if you expect it to be machine readable!

@dharesign
Copy link
Contributor

The spec is here: https://tc39.es/ecma262/#sec-date.parse

It specifically says:

The function first attempts to parse the String according to the format described in Date Time String Format (21.4.1.18), including expanded years. If the String does not conform to that format the function may fall back to any implementation-specific heuristics or implementation-specific date formats.

and:

However, the expression

Date.parse(x.toLocaleString())

is not required to produce the same Number value as the preceding three expressions and, in general, the value produced by this function is implementation-defined when given any String value that does not conform to the Date Time String Format (21.4.1.18)

I don't know if there's a more canonical reference for what implementation-defined strings v8 supports other than this comment in its source:
https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/date/dateparser-inl.h#L23-L69

It does seem more of a coincidence that Date.parse(d.toLocaleString("en-US")) works, rather than a guarantee. But given it does work, and v8 specifically intended to delegate to icu for determining whitespace, it seems reasonable to continue the status quo.

In general though, if you want guaranteed-to-work round-trip date formatting and parsing, then you would want to use the ISO 8601 format given by toISOString().

@dharesign
Copy link
Contributor

This was fixed upstream:

https://bugs.chromium.org/p/v8/issues/detail?id=13494

@srl295
Copy link
Member

srl295 commented Nov 21, 2022

This was fixed upstream:

https://bugs.chromium.org/p/v8/issues/detail?id=13494

FWIW change LGTM

targos added a commit to targos/node that referenced this issue Nov 22, 2022
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: nodejs#45171
@tukusejssirs
Copy link

tukusejssirs commented Nov 23, 2022

I just want to leave a note that new Date(new Date().toLocaleString('en-US')) works as expected int node@19.0.1, but not in node@19.1.0.

targos added a commit to targos/node that referenced this issue Nov 23, 2022
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: nodejs#45171
nodejs-github-bot pushed a commit that referenced this issue Nov 24, 2022
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: #45171
PR-URL: #45573
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos added a commit that referenced this issue Nov 24, 2022
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: #45171
PR-URL: #45573
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
ruyadorno pushed a commit that referenced this issue Nov 24, 2022
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: #45171
PR-URL: #45573
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@Hexagon
Copy link

Hexagon commented Nov 27, 2022

This is still a problem in 19.1

image

@michelpromonet
Copy link

michelpromonet commented Nov 29, 2022

Hi,

It seems working on 19.0

echo "console.log(new Date((new Date(1000000000000)).toLocaleString()))" | docker run --rm -i node:19.0

gives

2001-09-09T01:46:40.000Z

But not on latest/19.1

echo "console.log(new Date((new Date(1000000000000)).toLocaleString()))" | docker run --rm -i node:19.1

fails with

Invalid Date

Then it seems fixed and re-broken ?

@dharesign
Copy link
Contributor

It was never an issue in official 19.0.x releases. It was broken in 19.1.0 when Node switched to ICU 72. It is subsequently fixed in 19.2.0 which cherry-picked the v8 commit that fixes it.

The original report here of it breaking against 19.0.0 must have been linking against an external ICU which was ICU 72.

@stalkerg
Copy link

Unfortunately, it's a default way to change the timezone for the date https://stackoverflow.com/questions/10087819/convert-date-to-another-timezone-in-javascript and many libraries use it.
Do you have a better way?

@srl295
Copy link
Member

srl295 commented Dec 28, 2022 via email

@stalkerg
Copy link

@srl295 indeed, but I don't know a better way now. A very popular library like dayjs uses it. Will be good to provide a better solution and add it to stack overflow.
I know it's not directly a nodejs issue but because it's so popular will be better to do something.

danielleadams pushed a commit that referenced this issue Jan 3, 2023
Original commit message:

    [intl] Enhance Date parser to take Unicode SPACE

    This is needed to prepare for the landing of ICU72.
    Allow U+202F in the Date String, which the toLocaleString("en-US")
    will generate w/ ICU72.

    Bug: v8:13494
    Change-Id: I41b83c4094ce3d0737a72dcd6310b52c68fdcdca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4027341
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Jungshik Shin <jshin@chromium.org>
    Commit-Queue: Frank Tang <ftang@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#84308}

Refs: v8/v8@2ada52c
Fixes: #45171
PR-URL: #45573
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
kazupon added a commit to intlify/vue-i18n that referenced this issue Jan 12, 2023
kazupon added a commit to intlify/vue-i18n that referenced this issue Jan 12, 2023
…Format components on SFC template and JSX/TSX (#1310)

* fix: support type inference of Translation, NumberFormat and DatetimeFormat components on SFC template and JSX/TSX

* fix: timezone setting

* fix: pinned node v18 minor version

* fix

* fix: disalbe node v18

related: nodejs/node#46123
related: nodejs/node#45171
aruniverse referenced this issue in iTwin/itwinjs-core Jan 31, 2023
* Update template path

* fix use of template

* clean up variables

* Regression test updates

* Fix node version of integration tests

* Fix regression testing machine pool

* Fix uses of fs.rmSync for Node 12 compatibility

* Do not limit the max parallel runs

* rush change

* Fix regression integration test on Mac and Windows

* fix issues with the params passed to removeSync

* fix lint rules

* Only run regression on 12.x as 12.22 is the latest minor version

* set timeout to an hr, run appui tests in node12, ?? not added til node14

* hacky fix for https://github.com/nodejs/node/issues/45171\#issuecomment-1290833591 until patched in node 18

* rush change

* fix more tests

* rush change

* use || because ?? not supported til node14. again

---------

Co-authored-by: Arun George <aruniverse@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. i18n-api Issues and PRs related to the i18n implementation. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants