nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.8k stars 29.7k forks source link

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

Closed ghiscoding closed 7 months ago

ghiscoding commented 11 months ago

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 commented 11 months ago

cc @richardlau @srl295

xfournet commented 11 months ago

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

xfournet commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

xfournet commented 11 months ago

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

srl295 commented 11 months ago

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 commented 11 months ago

See https://github.com/nodejs/node/issues/42440#issuecomment-1284608792 for more discussion

climba03003 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

new Date('2023-12-08T00:00:00.000Z').toLocaleDateString('sv')
srl295 commented 11 months ago

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

srl295 commented 11 months ago

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

srl295 commented 11 months ago

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 commented 11 months ago

This change also included in the upcoming node@20.11.0 proposal

Not anymore, this change is not included now.

srl295 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

srl295 commented 10 months ago

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

ghiscoding commented 7 months ago

[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 commented 7 months ago

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