search

formkit / tempo

📆 Parse, format, manipulate, and internationalize dates and times in JavaScript and TypeScript.
https://tempo.formkit.com
MIT License
2.37k stars 33 forks source link

Narrow validOffset to allow only ISO8601 values #11

Closed mdbudnick closed 9 months ago

mdbudnick commented 9 months ago

Proposed fix for https://github.com/formkit/tempo/issues/10 by narrowing the offset regex in parse, and validating directly against all known UTC offset strings directly in validOffset instead of using a regex.

Adds a test that would pass with the old validOffset.

vercel[bot] commented 9 months ago

@mdbudnick is attempting to deploy a commit to the Formkit Team on Vercel.

A member of the Team first needs to authorize it.

justin-schroeder commented 9 months ago

Thanks for the PR! The list of offsets has 2 problems I can think of off the top of my head. First off, it is not comprehensive as there are also 15 minute offsets and these offsets are not exclusively UTC offsets, but also represent the offset between any two given timezones. Second, the weight of the offset table is something worth avoiding. Open to other suggestions!

mdbudnick commented 9 months ago

Fair points on the static map, the more I researched the more it ended up being.

I've changed it to just having a narrower regex. I don't think there can be an offset greater than 12, so the first digit can only be 0 or 1 and, obviously, over 59 minutes would be 1 hour. It's not very satisfying, but better than it is now.

justin-schroeder commented 9 months ago

Hmm. I think, in general, the offsets validation should not try to match it to any given location or locale. That’s not the intention of the function — it is, more or less, try to ensure that it is in the format: [+-]\d{4} — in otherwords can it be parsed and applied.

The reality is the world is a bizarre place and if someone wants to calculate the offset between two different timezones and validate their result using this function it should work. that could mean an offset as big as +-2445. So I guess I would hear a case for something more like /^[+-](?:[0-2][0-4]|[0-9)[0-6][0-9]/. That said I’m also not entirely sure what actual problem we are solving by making this change?

mdbudnick commented 9 months ago

The problem is clear from my title, description, solution, and test. The regex is considering 39 hours in a day and 69 minutes in an hour. All I will say is, best of luck to you and your project.