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 Fails to Parse Time #1382

Closed
derekhubbard opened this issue Feb 17, 2023 · 18 comments
Closed

DateTime.fromFormat Fails to Parse Time #1382

derekhubbard opened this issue Feb 17, 2023 · 18 comments

Comments

@derekhubbard
Copy link

derekhubbard commented Feb 17, 2023

Describe the bug
Using DateFormat.fromFormat to parse a time value fails to parse the time successfully, resulting in an invalid DateTime object.

This worked as expected until updating my browser from Chrome 109 to 110.

Parsing the same time value in Safari 16.3 and Firefox 110 works as expected.

To Reproduce

import { DateTime } from 'luxon';

const testTime = DateTime.fromFormat('1:07 AM', 't');

console.log('DateTime.isValid', testTime.isValid);
console.log('DateTime.invalidReason:', testTime.invalidReason);
console.log('DateTime.invalidExplanation:', testTime.invalidExplanation);

The output of the console logs above are:

DateTime.isValid false
DateTime.invalidReason: unparsable
DateTime.invalidExplanation: the input "1:07 AM" can't be parsed as format t

Actual vs Expected behavior
The time format in the example above (1:07 AM) should be successfully parsed as a valid DateTime object.

Desktop (please complete the following information):

  • OS: OSX
  • Browser: Chrome 109/110, Safari 16.3, Firefox 110
  • Luxon version: 3.2.1
  • Your timezone: "America/New_York"

Additional context
It's not clear yet whether this is Chrome bug or an issue with Luxon, but starting here in the hopes that you will be able to help me research it more. Thanks in advance!

@derekhubbard
Copy link
Author

derekhubbard commented Feb 17, 2023

It sounds like others are experiencing this issue when updating to Node 18 as well: #1364 (comment)

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

Same issue once upgrade browser.

Please check this output, very strange
image

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

Follow up: It turns out, Luxon only accept white space of ascii code 8239, not ascii code 32. Because Luxon use literal value to generate Regex value instead of using \s
image

@diesieben07
Copy link
Collaborator

This is behaving exactly as described in the documentation, imho.
tt is described as the "localized time with seconds". Localized meaning "whatever the current environment says it should look like according to the locale". If the environment (i.e. Chrome in this case) says there should be Unicode 8239 there, then Unicode 32 is not valid.
If you want a rigorous, specific format, you need to build it up yourself from non-localized tokens. tt and other localized tokens are implementation dependent.

This is also mentioned in the documentation:

If the string is typed out by a human, it may not conform to the format you specify when asking Luxon to parse it. Luxon is quite strict about the format matching the string exactly.

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

@diesieben07 It's a breaking change nodejs/node#46123

I think it should be handled by this lib, rather than users.

One more thing, the output of API in different browser should be same. It should be handled by this lib.

Edge:
image

Chrome:
image

@diesieben07
Copy link
Collaborator

diesieben07 commented Feb 20, 2023

toLocaleString is specifically also dependent on the underlying implementation. That allows Luxon to be smaller in size than e.g. moment, which has to ship all the locale data. All that toLocaleString guarantees is that it will try to show the date / time in whatever locale you have specified. The exact format is not defined and can change (like you are observing now).

If you rely on specific output from toLocaleString you are imho misusing the locale API. This also applies to using the locale-dependent format tokens like tt. If you want to parse a string like 10:30 pm, you should be using the format h:m a, which specifically says "I want a space, not a narrow width space". The only use-case for parsing with tt is if you are parsing the output of toLocaleString from some horrible legacy API that does not use ISO format.

So: This is not something to be "handled". You are using the wrong tool.

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

@diesieben07 Thanks for the reply. But if you use h:m a, how to make sure to use correct regional format. Such as en-US is 11:45:00 PM ,and en-IN is 11:45:00 pm, while jp is 23:45:00

@diesieben07
Copy link
Collaborator

You can't have both. Either you want the local format, then you have to accept it as is from the underlying system (including the fact that ICU now says that a narrow space should be used) or you have to provide your own formats.

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

No, I don't agree. It's ridiculous cannot have both locale and regional format.

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

Is there any maintainer active here?

@diesieben07
Copy link
Collaborator

I'd be curious how you would like Luxon to handle this instead of the current system.

@gBusato
Copy link

gBusato commented Feb 26, 2023

Didn't had the time to wait for a merge, there I have replace my string : "9:23 AM".replace(/\s/g, '\u202F') and now it work

@diesieben07
Copy link
Collaborator

@shinnqy @derekhubbard Would an option looseWhitespace to DateTime.fromFormat solve this issue for you? This option would replace any whitespace-literals given by the browser for the locale-dependent format with a \s in the regex (not quite, because we don't really want to allow \n or \r). In that case any whitespace in the input can stand-in for any whitespace in the required format.

@diesieben07
Copy link
Collaborator

#1369 has been merged. The next version of Luxon will treat non-breaking white-space equally when parsing macro-tokens like t.

@gtamhan
Copy link

gtamhan commented Mar 1, 2023

@diesieben07 Also running into the same issue after updating Chrome 109 to 110. When will the next version be released?

@diesieben07
Copy link
Collaborator

That would be up to @icambron, I don't have permissions to do releases.

@ccmetz
Copy link

ccmetz commented Mar 4, 2023

@diesieben07 I understand that this will be fixed once the next version is released but just out of curiosity, why does DateTime.now().toFormat('t') not return a string using the narrow no-break space character that chrome expects? I would understand why this doesn't work on Chrome:

DateTime.fromFormat('1:30 AM', 't');

but why wouldn't this work?

let time = DateTime.now().toFormat('t');
let datetime = DateTime.fromFormat(time, 't');

I guess I would expect that since I formatted the datetime on Chrome, it would use that narrow no-break space character character that it is expecting when parsing the string using fromFormat?

@diesieben07
Copy link
Collaborator

@ccmetz My guess is that your browser is on a different locale than en-US. fromFormat defaults to en-US parsing, not the browser locale.

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