Polyfill: Don't allow offsets of the form +00:0000 #3196
Conversation
This offset is not allowed because the separator for hours/minutes and the separate for minutes/second must be consistent. To make this change, I refactored most of the regexps used for time/date parsing to use named groups. This was to make the code easier to follow The change in semantics is found in the `offsetWithParts` and `offset` regexes, which previously made the ':' separator optional in both of these cases, allowing +00:0000 or +0000:00. The fix is to use a disjunction (introducing new regexps optionalMinSecWithSep and optionalMinSecNoSep), to enforce that the separator or lack thereof is consistent.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
==========================================
+ Coverage 96.74% 96.75% +0.01%
==========================================
Files 22 22
Lines 10348 10387 +39
Branches 1859 1859
==========================================
+ Hits 10011 10050 +39
- Misses 287 289 +2
+ Partials 50 48 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Wow, this is a long-overdue refactor to get rid of those horrible +(match[27] || match[35] || 0)` lines and replace them with something readable. Thanks!
I will note that it relies on duplicate named capture groups which was only shipped in browsers in 2024. Historically we did not want to use features in the polyfill that were too recent, because we didn't want people going to the browser playground and getting an error. But I don't think that's much of a concern anymore now that browsers are actually shipping Temporal, so no need to change that, just noting it for future reference.
Other that that, I had a few comments.
| const fraction = /(\d+)(?:[.,](\d{1,9}))?/; | ||
| const fraction1 = /(\d+)(?:[.,](\d{1,9}))?/; |
There was a problem hiding this comment.
I'd prefer numberWithOptionalFraction or something like that, to distinguish it better from the fraction introduced above.
| const second2 = /(?<second>\d{2})/; | ||
| const fraction = /(?:[.,](?<fraction>\d{1,9}))/; | ||
| const secondsfraction = new RegExpCtor(`(?:${sep.source}?${second2.source}${fraction.source}?)`); | ||
| const millismicrosfraction = /(?<millis>\d{2})(?:(?<micros>\d{2}))(?:[.,](?<nanos>\d{1,9}))?/; |
There was a problem hiding this comment.
The names don't seem right here, it's 3 digits each of ms and µs and they should come after the decimal separator. Is it possible these capture groups should actually be referring to hours and minutes?
| const optionalMinSecWithSepNoCapture = new RegExpCtor( | ||
| `(?:${sep.source})(?:${minute.source})(?:${sep.source}(?:${second.source}${subseconds.source}?))?` | ||
| ); | ||
| const optionalMinSecNoSepNoCapture = new RegExpCtor(`(?:${minute.source})(?:${second.source}${subseconds.source}?)?`); | ||
| const optionalMinSecNoCapture = new RegExpCtor( | ||
| `${optionalMinSecWithSepNoCapture.source}|${optionalMinSecNoSepNoCapture.source}` | ||
| ); | ||
| export const offsetWithParts = new RegExpCtor( | ||
| `^(?<sign>${sign.source})(?<hour>${hour.source})(?:${optionalMinSec.source})?$` | ||
| ); | ||
| export const offset = new RegExpCtor( | ||
| `(?<offset>(?:${sign.source})(?:${hour.source})(?:${optionalMinSecNoCapture.source})?)` |
There was a problem hiding this comment.
It seems possible that we could just drop the non-capturing alternatives since we are now using named capture groups instead of numbered ones, so extra captures won't mess up the numbering? Or are they necessary to distinguish the hour/minute/second captures in offsets from the hour/minute/second captures in time parts?
2 participants
This offset is not allowed because the separator for hours/minutes and the separate for minutes/second must be consistent.
To make this change, I refactored most of the regexps used for time/date parsing to use named groups. This was to make the code easier to follow The change in semantics is found in the
offsetWithPartsandoffsetregexes, which previously made the ':' separator optional in both of these cases, allowing +00:0000 or +0000:00. The fix is to use a disjunction (introducing new regexps optionalMinSecWithSep and optionalMinSecNoSep), to enforce that the separator or lack thereof is consistent.