moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.4k stars 729 forks source link

DateTime.fromFormat is not able to correctly parse the result of DateTime.toFormat() #857

Open spooky opened 3 years ago

spooky commented 3 years ago

The following code results in an invalid DateTime:

const x = new Date();
DateTime.fromFormat(DateTime.fromJSDate(x).toFormat('D T'), 'D T')

error on the resulting DateTime object:

invalid: {…}
​​explanation: "you specified 20 (of type number) as a month, which is invalid"
​reason: "unit out of range"

I'm using firefox 84.0.2 on windows 10, lang in browser set to en-GB, windows region format set to Polish, windows display language set to en-GB, windows apps and websites land set to en-US.

icambron commented 3 years ago

Odd. What does DateTime.local().locale return in this environment?

spooky commented 3 years ago

'en-GB'

dan-overton commented 3 years ago

Managed to reproduce this at this codesandbox (with the system locale being en-GB).

luxon.DateTime.fromFormat('14/02/2021 21:00', 'D T') is invalid but luxon.DateTime.fromFormat('14/02/2021 21:00', 'D T', {locale: 'en-GB'}) parses correctly.

The issue is that, in the browser, Settings.defaultLocale. is null. This makes fromFormat default to parsing in en-US. which makes the day/month the wrong way round in the parsing.

In using fromFormat I'd expect it to use the first available of:

  1. An explicitly provided locale in opts
  2. Settings.defaultLocale
  3. The system locale
  4. en-US

But 3 / 4 appear to be switched as it stands.

dan-overton commented 3 years ago

I've managed to get a unit test for this up and running (by moving systemLocale to a separate module and mocking it). Flipping defaultToEN in fromFormat to false makes the test pass, and all other tests continue to pass.

I'm a little nervous about it though, as I assume that flag was explicitly set to true for a reason.

icambron commented 3 years ago

Forcing the parse to default to EN is indeed on purpose. Imagine you're building a web app and you're requesting data off of some backend, and it formats dates like, say, 05/15/21 9:15am, which you parse with fromFormat. You don't want your parsing code to break because the user changes their locale.

However, the localized macro tokens really should localized with the default locale. I had thought they did

spooky commented 3 years ago

I confirm that passing the locale explicitly in the options resolves this issue. The reason I reported it though is that when running on defaults for both fromFormat and toFormat, I'd expect this to use the same locale whereas it seems that's not the case.

dan-overton commented 3 years ago

I think if I was expecting US formatted dates from a backend, I'd expect to have to specify en-US explicitly in fromFormat. That is, I'd find having to do that that less unintuitive than this bug.

If that's the expected behaviour though, I guess this one can be closed?

patriceo commented 2 months ago

Hello I'm facing the same issue with luxon 3.3.0. It can easily be reproduced.

        const fmt = 'D t ZZZZ';
        const now = DateTime.now();
        const formattedNow = now.toFormat(fmt, {locale: 'en-US'});
        const parsedNow = DateTime.fromFormat(formattedNow, fmt, {locale: 'en-US'});
        console.info( parsedNow.invalidReason ); // --> unparsable
diesieben07 commented 2 months ago

@patriceo ZZZZ is not a valid parsing token. Not all tokens that can be formatted can be parsed, mostly due to browser limitations. In this case, there is no way for Luxon to parse a time zone, because there is no API to do this in the browser.

patriceo commented 1 month ago

Hi @diesieben07, thanks for pointing this out, I missed it.

Do you know if there is any plan to change this implementation so the two methods get symmetrical? I didn't check the implementation but the standard Date class can parse some formatted dates with explicit timezone https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse

I don't see any option to parse a DateTime with time-zone with the current version.

Thanks!

diesieben07 commented 1 month ago

Date.parse can only officially parse a simplified ISO format:

Only the date time string format is explicitly specified to be supported. Other formats are implementation-defined and may not work across all browsers.

What it can parse is offsets, but Luxon already offers this (via the Z, ZZ, ZZZ parsing tokens).

diesieben07 commented 1 month ago

To add to that: The problem with parsing time zones is that they are not unique or canonical (except for IANA names, which Luxon can parse via the z token).