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

Inconsistent DateTime format when using toLocaleString #1333

Closed
bliu13 opened this issue Nov 18, 2022 · 10 comments
Closed

Inconsistent DateTime format when using toLocaleString #1333

bliu13 opened this issue Nov 18, 2022 · 10 comments

Comments

@bliu13
Copy link

bliu13 commented Nov 18, 2022

I don't know if this is related to something on my MacBook, but for some reason, my tests started failing with this:

const timestamp = "2022-05-11T23:23:00-07:00";  // Example timestamp string
DateTime.fromISO(timestamp, { zone: 'America/Los_Angeles' }).toLocaleString(DateTime.DATETIME_FULL);

On any other machine, this is the sample formatted output, but should be correct according to the documentation:
May 12, 2022, 7:00 AM PDT

On my Macbook, I would get this instead:
May 12, 2022 at 7:00 AM PDT

  • OS: MacOS 13.0.1
  • node.js version: 19.1.0
  • Luxon version: 3.1.0
  • Timezone: America/Los_Angeles
@dobon
Copy link
Contributor

dobon commented Nov 19, 2022 via email

@bliu13
Copy link
Author

bliu13 commented Nov 19, 2022

@dobon, I just tried out node.js version 16 and 18 as well, but the results were the same.

@icambron
Copy link
Member

I'm speculating, but I've always assumed that this kind of thing is a change in the minor version of Node, across all the supported major versions. They update the ICU data in all those majors and we get string changes in all of them

@bliu13
Copy link
Author

bliu13 commented Nov 21, 2022

@icambron, I went and checked the node versions on the different machines to get some more data:

Tests passing for:

  • Build system was using 16.13.1
  • Coworker was using 18.0.0

I made some changes to the build system to use the current up to date LTS version which is at 18.12.1 and the build system is also now consistently failing on the same tests. I am inclined to believe your hunch is right about something related to Node changing something. What do you think should be done about this?

@bliu13
Copy link
Author

bliu13 commented Nov 21, 2022

I tried out a couple of more tests between node versions.
I configured the build system to use:

  • 18.2.0
  • 18.1.0
  • 18.0.0

I tried 18.2.0 because this was the only release in version 18 that have changes related to ICU Locale and I am seeing some bizarre results. All three versions are generating this format:
May 12, 2022 at 7:00 AM PDT.

I am not really sure what's going on, but I was expecting 18.2.0 to cause the breakage, but using 18.0.0 is also showing breakages.

@diesieben07
Copy link
Collaborator

Personally I do not think testing against the output of toLocaleString is useful. toLocaleString by definition is implementation dependent. If you need a format that must be absolutely consistent, you cannot use toLocaleString, you need to use toFormat or toLocaleParts and construct the string yourself.

@icambron
Copy link
Member

I agree that users of Luxon shouldn't be testing against the output of toLocaleString; they should merely test that the inputs to Luxon are what they think they should be.

However, Luxon itself needs to test, at the very least, that the inputs it sends to Intl are right. It doesn't currently have a good way to do that, so we test the output of toLocaleString too. A refactor that gave us an Intl mock that we could assert against would fix this problem in Luxon's own tests.

@inomn
Copy link

inomn commented Dec 21, 2022

Hey everyone. Tried out my test on Node19 and here's what I got (works well on node 16, 17, 18):

Screenshot 2022-12-21 at 15 41 58
Screenshot 2022-12-21 at 15 42 38

In Node 19 space character code is 8239 and in "older" Node, including current LTS it's 32

Any ideas on a workaround?

Same space issue with both
DateTime.fromISO(date).toLocaleString(XXX)
and DateTime.fromISO(date).toFormat(XXXX, { locale })

@bliu13
Copy link
Author

bliu13 commented Jan 7, 2023

@inomn Yeah, this is wild. I ran into the same problem where the space character is different. I took a look at the byte representation and prior to this, the encoded space character was 20, but now it's e2 80 af. I actually needed to validate a message and the date time just happened to be in the message. Since the consensus is to avoid using snapshot tests with date time, it might be better for me to just validate the messages in chunks.

It is related to this:
nodejs/node#45068

Since these issues are not caused by luxon, I think this issue should be closed.

@bliu13 bliu13 closed this as completed Jan 9, 2023
@raing3
Copy link

raing3 commented Mar 13, 2023

The Node behaviour change looks to have been reverted in 18.15: nodejs/node#46646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants