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

regression - toLocaleString as UTC TZ is displayed as short date (1 digit) on Node 21 but (2 digits) on Node 18 #51090

Closed
ghiscoding opened this issue Dec 7, 2023 · 23 comments
Assignees
Labels
i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency.

Comments

@ghiscoding
Copy link

ghiscoding commented Dec 7, 2023

Version

21.4.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

No response

What steps will reproduce the bug?

Running the following script with today's date (2023-12-07) is displayed differently on Node 21 than it is on Node 18, this seems like a regression and I caused some failing unit tests in my CI because it expected a date format of YYYY-MM-DD but that is no longer true in Node 21.

This simple script

const formatted = new Date().toLocaleString('sv-SE', {
  timeZone: 'UTC'
});
    
console.log(formatted);

is displaying the following

Node 21

notice the December 7th is displayed as 7 instead of the expected 07

2023-12-7
Node 18
2023-12-07

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

every time I tried on Node 21.4.x

What is the expected behavior? Why is that the expected behavior?

It should provide an ISO formatted date

What do you see instead?

December 7th is displayed as 7 instead of 07, below is a print screen of the simple script that I ran on Node 18 and 21

image

Additional information

This date format script is used by conventional-changelog library here and the unit tests I have in Lerna-Lite failed only on Node 21 in this GitHub Action CI workflow because it expected a format of YYYY-MM-DD but it instead provided an unexpected format of YYYY-MM-D.

The reason my unit tests failed is because, the tests are using a serializer replace any date to a static 'YYYY-MM-DD' text that the tests can compare test snapshot, but it started failing on Node 21 because the regex no longer found the expected date format. For now I changed the regex from formattedDate.replace(/\(\d{4}-\d{2}-\d{2}\)/g, '(YYYY-MM-DD)') to formattedDate.replace(/\(\d{4}-\d{1,2}-\d{1,2}\)/g, '(YYYY-MM-DD)') and my tests are now passing again but it's still seem to be regression in Node 21

Note that I'm on Windows but my CI is running on Ubuntu, so it's not an OS specific issue

As for the use of sv-SE locale, it looks like it's because Sweden has a fixed format that is unlikely to change and is the nearest to the ISO format as explained in this Stack Overflow answer

@marco-ippolito marco-ippolito added the icu Issues and PRs related to the ICU dependency. label Dec 8, 2023
@marco-ippolito
Copy link
Member

cc @richardlau @srl295

@xfournet
Copy link

xfournet commented Dec 8, 2023

I also have this regression when updating from 21.2.0 to 21.3.0 or 21.4.0

@xfournet
Copy link

xfournet commented Dec 8, 2023

Repro case:

node -e "const locale = 'sv'; console.log({version: process.version, locale, str: new Date().toLocaleString(locale)})"

Results:

{ version: 'v21.2.0', locale: 'sv', str: '2023-12-08 15:26:21' }
{ version: 'v21.3.0', locale: 'sv', str: '2023-12-8 15:26:09' }

@climba03003
Copy link
Contributor

The changes related to updating ICU to icu@74.1
Inside the PR, we can see the test is breaking. Is it expected to break?

This change also included in the upcoming node@20.11.0 proposal

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

It's not a regression. Date formats change in response to changes in cultural and linguistic preferences.

@xfournet
Copy link

It should at least be mentionned as a breaking change in the release note

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

It should at least be mentionned as a breaking change in the release note

Data changes constantly. Look at https://www.unicode.org/cldr/charts/44/delta/index.html and pick any locale. toLocaleString() is completely inappropriate as an API to expect a constant format. It should display what's correct for Swedish, which might change over time.

Node 18 is CLDR 43.1, Node 21 is 44.

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Dec 12, 2023
@srl295
Copy link
Member

srl295 commented Dec 12, 2023

See #42440 (comment) for more discussion

@climba03003
Copy link
Contributor

climba03003 commented Dec 12, 2023

I know that ICU changes very often, and people are wrongly expect the output should be consistence inside the major version.
But as a developer, it would be great to know that there will be changes exist inside locale (maybe a notable changes label).

As per the delta charts https://www.unicode.org/cldr/charts/44/delta/sv.html
It stated there is no changes on the date format.
No idea why it is changed, should be upstream issue.

Randomly pick one that has date format changes for reference
https://www.unicode.org/cldr/charts/44/delta/bg.html

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

I know that ICU changes very often, and people are wrongly expect the output should be consistence inside the major version. But as a developer, it would be great to know that there will be changes exist inside locale (maybe a notable changes label).

There are always changes.

As per the delta charts https://www.unicode.org/cldr/charts/44/delta/sv.html It stated there is no changes on the date format.

That's true, so I'm not sure where the format came from. I'll see.

@xfournet
Copy link

It should at least be mentionned as a breaking change in the release note

Data changes constantly. Look at https://www.unicode.org/cldr/charts/44/delta/index.html and pick any locale. toLocaleString() is completely inappropriate as an API to expect a constant format. It should display what's correct for Swedish, which might change over time.

Node 18 is CLDR 43.1, Node 21 is 44.

You're right. Moreover this API shouldn't be expected to produce consistent result, and it's clearly stated in a note in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString
cf: You should not compare the results of toLocaleString() to static values.

So no breaking change here.

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

this may actually be a v8 bug, ICU 74.1 gives:

$ env ICUDATE_OPTS='-L sv -s'  make check
DYLD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw:$DYLD_LIBRARY_PATH  ./icudate -L sv -s
2023-12-12 08:20

also

$ node
Welcome to Node.js v21.4.0.
Type ".help" for more information.
> new Intl.DateTimeFormat(['sv']).resolvedOptions();
{
  locale: 'sv',
  calendar: 'gregory',
  numberingSystem: 'latn',
  timeZone: 'America/Chicago',
  year: 'numeric',
  month: 'numeric',
  day: 'numeric'
}

but

$ node
Welcome to Node.js v18.19.0.
Type ".help" for more information.
> new Intl.DateTimeFormat(['sv']).resolvedOptions();
{
  locale: 'sv',
  calendar: 'gregory',
  numberingSystem: 'latn',
  timeZone: 'America/Chicago',
  year: 'numeric',
  month: '2-digit',
  day: '2-digit'
}

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

by the way a test case that works past Dec 9th is

new Date('2023-12-08T00:00:00.000Z').toLocaleDateString('sv')

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

Chromium latest doesn't seem to be on ICU 74 yet.

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

Upstream ICU bug https://unicode-org.atlassian.net/browse/ICU-22575 to be fixed in 74.2

@srl295
Copy link
Member

srl295 commented Dec 12, 2023

I stand by what i said about this being a very bad idea for code. Perhaps the bug will flush out more of these incorrect usages.

However, this particular bug seems to be an ICU bug. See https://bugzilla.mozilla.org/show_bug.cgi?id=1864612 for the parallel in Firefox.

@UlisesGascon
Copy link
Member

This change also included in the upcoming node@20.11.0 proposal

Not anymore, this change is not included now.

@srl295
Copy link
Member

srl295 commented Dec 13, 2023

@ghiscoding thank you for raising this issue in time to keep the bugs update out of 20.11.0. (By the way, the bug affected more than just this single date format in Swedish. I don't have a comprehensive list.)

Meanwhile, ICU 74.2 is coming real soon now.

@ghiscoding
Copy link
Author

You're more than welcome, this is my first reported issue in NodeJS project and I'm glad it got a lot of attentions from you guys. Thank you so much for supporting this great project :)

@srl295 srl295 self-assigned this Dec 13, 2023
@srl295
Copy link
Member

srl295 commented Dec 13, 2023

@ghiscoding I have to say that NodeJS is a great project with a great community. Welcome!

@srl295
Copy link
Member

srl295 commented Dec 22, 2023

update: conventional-changelog has fixed their code to no longer misuse sv-SE 🎉

@ghiscoding
Copy link
Author

[v20.x backport] deps: update icu to 74.2

Landed in e8f5735...4ee7f29.

@srl295 I was assuming that the quoted commit might closes this issue but then realized it's for Node 20.x, was this commit also pushed to 21.x?

@aduh95
Copy link
Contributor

aduh95 commented Apr 3, 2024

I think the fix has landed on all active release lines, and can be closed now.

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. icu Issues and PRs related to the ICU dependency.
Projects
None yet
Development

No branches or pull requests

7 participants