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 & regex match failed once upgrade to latest version of Edge and Chrome #1384

Closed
shinnqy opened this issue Feb 20, 2023 · 10 comments

Comments

@shinnqy
Copy link

shinnqy commented Feb 20, 2023

Describe the bug
Once upgrade to the latest version of edge and chrome. DateTime.fromFormat failed, even the output generated by itself.

When I looked into this. I find the regex match failed.
image

To Reproduce
image

Actual vs Expected behavior
A clear and concise description of what you expected to happen.

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser: latest version of edge and chrome
  • Luxon version: 3.2.1
  • Your timezone: beijing/shanghai +8

Additional context
Add any other context about the problem here.

@shinnqy
Copy link
Author

shinnqy commented Feb 20, 2023

image

I think you also need to handle unicode 32. Why not using \s instead of \ ?

Because Luxon use literal value to generate Regex value instead of using \s. It will use unicode 8238 to match.
image

@tehamawirelessd4
Copy link

tehamawirelessd4 commented Feb 22, 2023

Same here. This was working:
const opts={locale: 'en-US', zone: 'local'}
result = DateTime.fromFormat(datetime, 'D tt', opts)

And now, it fails. There's an unprintable unicode character in the regex right before the AM/PM indicator. This makes the parse fail, returning invalid date with the same legacy date strings as last week. My legacy date strings look like this: '2/21/2023 11:01:28 PM'.

As a workaround, I now use:
result = DateTime.fromFormat(datetime, 'D hh:mm:ss a', opts)

Which does not put the unicode character into the regex

Have also confirmed this is not an issue in Safari

@dasa
Copy link
Contributor

dasa commented Feb 22, 2023

This may be fixed by #1369

@shinnqy
Copy link
Author

shinnqy commented Feb 23, 2023

@dasa Thanks for the info. This PR created 3 weeks ago. Would you please help review it and merge it? I see you are contributor.

@dasa
Copy link
Contributor

dasa commented Feb 23, 2023

I see you are contributor.

That just means I've had PRs merged in the past: https://github.com/moment/luxon/commits?author=dasa

I'm not able to merge any PRs myself.

@dave0783
Copy link

Any word on when this can be merged We are having this issue as well.

@joepwijnhoven
Copy link

+1

1 similar comment
@geertjansen
Copy link

+1

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

@RonNabuurs
Copy link

Just confirmed that this is also an issue in safari.

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

8 participants