Fix support for Node.js 18.13+

[mohd-akram]

Fixes #1364

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mohd-akram / name: Mohamed Akram (b767694, 88f3ba5)

[cscrosati]

/cc @icambron
It appears this fix could help folks that depends on Luxon for date format parsing in Chrome/Edge v110+ and Node v18.13+.

Can it be merged and released as a minor update if you agree?

Should fix:
#1364 and #1382

[jdom]

This started affecting our customers that use Chrome/Edge. Any chance we can expedite this review?

[diesieben07]

I don't think this should be done always, it should be opt-in. An option like looseWhitespace for fromFormat would be good.
Additionally, I don't think we want to allow new-lines (which \s covers).

[diesieben07]

Some clarification of the above:

• By default fromFormat should keep behaving as it does now.
• An option should be added to fromFormat like looseWhitespace, which toggles an alternative behavior. In this alternative behavior, any whitespace given by the unterlying system for the "macro tokens" (e.g. tt for "local time") is mapped to [^\S\n\r] (match any whitespace except new lines). This way all whitespace is "treated equal" in terms of parsing.
• I don't think outputting of dates should be affected. The output of toLocaleString (and by extension the macro tokens) is by definition variable. If you need a consistent output, use toFormat without macro tokens.

[mohd-akram]

The docs actually say t is for localized time even when parsing, so it should be able to parse the localized output without any extra options. Also, new Date accepts a newline as space when passing it a localized string, so it makes sense to match that behavior.

[diesieben07]

The docs actually say t is for localized time even when parsing, so it should be able to parse the localized output without any extra options.

Correct, and so it does. But the localized output in this case contains a narrow width space, not a normal space. As such a normal space does not fit the format. This change makes parsing more lax, allowing any whitespace in place of where the localized time says a narrow width space should be.

Also, new Date accepts a newline as space when passing it a localized string, so it makes sense to match that behavior.

new Date() or more specifically Date.parse only guarantees support for ISO-8601. It will try to parse other formats, but doing so is implementation dependent and may not work at all. Attempting to parse a localized date with this method is futile (is 03-04-2022 March 4th or April 3rd?) and not what this method is for.

[diesieben07]

To clarify:

const dt = DateTime.now();
const roundtrip = DateTime.fromFormat(dt.toLocaleString(DateTime.TIME_SIMPLE), 't');

This always works. t parses whatever toLocaleString(DateTime.TIME_SIMPLE) would do - and that is implementation dependent.

[mohd-akram]

This change makes parsing more lax, allowing any whitespace in place of where the localized time says a narrow width space should be.

It still is a regular space in certain runtimes. Localized time is by definition lax. If one needs strict parsing they should use explicit tokens. Otherwise, the t token is virtually useless without your specified option since most will expect it to parse a regular space - and browsers were patched to do so.

[diesieben07]

I'm sorry. I think I was running down a path of "but the docs say so, so it must be done this way".
Thinking about it more broadly, you are correct. When parsing, white-spaces should be treated equally and in fact Luxon already does something similar.

One change I would suggest though is to make the regex not accept new lines, i.e. use [^\S\n\r] instead of \s. I don't think it makes sense to allow parsing

10:30
PM

[diesieben07]

Another note, I am not a fan of how special-cased this implementation is. I think it would be better, to handle the white-space normalization similar to how it is already done for the other parsing code in tokenParser.js. The literal method should be adjusted to normalize white-space, like oneOf already does.

[diesieben07]

Thank you for your contribution @mohd-akram and sorry for the back and forth.

[dasa]

It seems Node and the browsers are reverting the breaking change, but now at least Luxon is compatible either way 😄

[ranjan-purbey]

Would it be possible to backport this fix for luxon 1.x as well?

[icambron]

@ranjan-purbey no, pretty much not going to make Luxon 1.0 fixes. Note that upgrading is not usually very involved, so I encourage you to do that instead.