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

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

Open
spooky opened this issue Jan 19, 2021 · 12 comments

Comments

@spooky
Copy link

spooky commented Jan 19, 2021

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
Copy link
Member

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

@spooky
Copy link
Author

spooky commented Jan 25, 2021

'en-GB'

@dan-overton
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

icambron commented Feb 15, 2021

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
Copy link
Author

spooky commented Feb 19, 2021

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
Copy link
Contributor

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
Copy link

patriceo commented Sep 4, 2024

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
Copy link
Collaborator

@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
Copy link

patriceo commented Sep 5, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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).

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

5 participants