Node 18.13 ICU 72 Version Breaking Change Date/Time format

[CreativeTechGuy]

Version

18.13.0

Platform

N/A

Subsystem

No response

What steps will reproduce the bug?

Related #45171. The whitespace before AM/PM in a time is now U+202F rather than a space character.

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

100% reproduction on the affected Node versions

What is the expected behavior?

Same behavior as previous. A space character to separate instead of unicode whitespace.

What do you see instead?

A NARROW NO-BREAK SPACE (U+202f) character instead of a space character.

Additional information

This is a breaking change, especially for automated tests. Upon upgrading to 18.13 our CI builds started to fail when asserting that a datetime string was what we expected. We eventually tracked this down to the change mentioned above. I understand the change itself isn't a bug, but the introduction of this in a minor LTS version seems like a bug.

While it's partially user error for many cases of misusing these formatted strings, it's unexpected for this to change in a minor version and a very difficult problem to track down.

[bnoordhuis]

Thanks for the report but it's been reported before and it's not considered a bug or backwards incompatible change in behavior. Tests that break because of this change are making bad assumptions about localized strings.

[kleinfreund]

Tests that break because of this change are making bad assumptions about localized strings.

That may well be, but it doesn’t change the fact that this change easily breaks all sorts of tests. One notable example are snapshot tests that contain date strings formatted in this manner. They will break with this change and aside from the consideration whether snapshot tests are a good idea or not, there can’t be any doubt that there was no way to make a "bad assumption about localized strings" when utilizing them.

For now, I’m restricting Node.js 18 to >=18 <18.13.0 via the engines field (and 18.12 for the .nvmrc) in my project to workaround the broken tests. Once 18.13 is distributed sufficiently across systems (I don’t get it yet on Ubuntu), I’ll change that to >=18.13.0 and regenerate the snapshots.

[markcarroll]

Thanks for the report but it's been reported before and it's not considered a bug or backwards incompatible change in behavior. Tests that break because of this change are making bad assumptions about localized strings.

@bnoordhuis how is this not a breaking change?

The behavior that has been the same for years has suddenly started causing regular expression matchers to fail. The \s whitespace matcher in regex has not been updated to include this character. If that change had accompanied this update that would been more excusable, but making a change that breaks existing behavior like this should not be in a minor release IMO.

At the very least it would be helpful if this were noted in the release docs so that folks trying to track down the problem have an easier change of finding it.

[bnoordhuis]

At the very least it would be helpful if this were noted in the release docs

It was. The update to ICU to 72.1 was called out as a notable change.

[CreativeTechGuy]

It was. The update to ICU to 72.1 was called out as a notable change.

I hope you can understand how to someone who isn't familiar with the nodejs codebase or these core libraries that it's unclear about this change. I spent ~3 hours digging through trying to root cause this and it wasn't until I found a thread about Node 19 which referenced ICU that I even was pointed in that direction. Before a week ago, I never had even heard of ICU or had any clue what it did. And even now, after looking at their repo, it's not clear that the time format was changed.

The ICU 72.1 changelog just says that they updated Unicode 15, CLDR 42, time zone data version 2022e, etc. Which those are now a bunch more things that you'd need to dig into to find where exactly this was changed. Honestly, I still haven't found the exact commit in any repo which changed this exact thing.

The Node changelog says: "deps: update ICU to 72.1". It would take a lot for a developer to make sense of that and I'm not even sure the steps they'd need to take to determine from that line that the above change was a result of that.

Out of curiosity, where exactly did the change take place? And was it called out in the ICU changelog somewhere that I missed?

[bnoordhuis]

The ICU update includes a CLDR update (from the Unicode consortium) which is the "human" representation of dates, numbers, etc. See #45945 (comment) for an example.

The "human" part is also why this isn't considered a breaking change or a regression. i18n is for humans, not machines.

[CreativeTechGuy]

The "human" part is also why this isn't considered a breaking change or a regression. i18n is for humans, not machines.

Can you confirm if Node supports automated testing use-cases? Automated tests are machines which imitate humans to ensure that something works as expected for a human. It's not like we are trying to parse the strings, just compare them to a known value to ensure our code works as expected. It seems like most people facing problems due to this (on this thread and others) is a result of automated tests finding a difference due to the space character.

[bnoordhuis]

I'm aware that's what people are running into and no, we don't consider that a bug. Tests shouldn't make assumptions about the format of localized outputs because those can change at a whim (or the Unicode Consortium's whim anyway.)

Automated tests are machines which imitate humans to ensure that something works as expected for a human.

Clearly not imitating humans well enough. In general we don't break down in the presence of a non-breaking space.

(Unmatched parentheses on the other hand...)

[kleinfreund]

The "human" part is also why this isn't considered a breaking change or a regression. i18n is for humans, not machines.

A software's tests are for humans, too. And they break now.

[bnoordhuis]

If you think that kind of argument is going to sway our position: it won't. If you're just trying to get the last word in: please don't.

[gu-stav]

While I understand your position, I just wanted to add this change started breaking snapshot tests for us at https://github.com/strapi/strapi. It came as a surprise in a minor version and took us some time to understand why test failures like the following started appearing because first we thought it might stem from the testing library (jest):

I believe this could have been communicated better or moved into a major version. To run snapshot tests across different node versions now requires mocking every instance of Date.