Refactor parseDate #487
Refactor parseDate #487
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #487 +/- ##
===================================
Coverage 100% 100%
===================================
Files 58 58
Lines 509 511 +2
Branches 83 85 +2
===================================
+ Hits 509 511 +2
Continue to review full report at Codecov.
|
src/index.js
Outdated
| reg[1], reg[2] - 1, reg[3] || 1, | ||
| reg[4] || 0, reg[5] || 0, reg[6] || 0, reg[7] || 0 | ||
| ) | ||
| if (typeof date === 'string' && !date.toLowerCase().endsWith('z')) { |
There was a problem hiding this comment.
Just noticed that endsWith is a new added method in js, with cost some more bytes than /.*[^Z]$/i.test in the final es5 code.
There was a problem hiding this comment.
Yes, but the concept is clearer. I also saved some letters on renaming reg[...] to d[...] multiple times and removing the let reg, wouldn't that even it out?
There was a problem hiding this comment.
reg will be minified in building I think, and endsWith becomes the added bytes.
There was a problem hiding this comment.
Funnily, this was never needed at all, there is a bug in the regex.
If you look at it
export const REGEX_PARSE = /^(\d{4})-?(\d{1,2})-?(\d{0,2})[^0-9]*(\d{1,2})?:?(\d{1,2})?:?(\d{1,2})?.?(\d{1,3})?$/you wonder: Why does it even match a z or Z it in the end? It is because the last dot in the regex is not escaped (the dot in part ?.?(\d{1,3})?), so that matches any character and therefore matches a z / Z.
If you change it to
export const REGEX_PARSE = /^(\d{4})-?(\d{1,2})-?(\d{0,2})[^0-9]*(\d{1,2})?:?(\d{1,2})?:?(\d{1,2})?\.?(\d{1,3})?$/i(note the backlash to escape the dot and the i flag), all tests are still green if we just do
if (typeof date === 'string') {
const d = date.match(C.REGEX_PARSE)
if (d) {
return new Date(d[1], d[2] - 1, d[3] || 1, d[4] || 0, d[5] || 0, d[6] || 0, d[7] || 0)
}
}So we even save a lot of bytes :-).
There was a problem hiding this comment.
Pushed that fix to PR.
There was a problem hiding this comment.
Ok I did, and even shortened it as /z$/i does the same (asserting it ends with a z, case insensitive). .*[^Z]$ was doing too much, no need to match any character 0-n times.
There was a problem hiding this comment.
/z$/i.test('z') doesn't make sense.
There was a problem hiding this comment.
Yes, I fixed it. I could just use the web view as I do not have the solution checked out right now, will crush those commits, sorry.
There was a problem hiding this comment.
Needs to be /[^Z]$/i.test(date), getting rid of .*.
There was a problem hiding this comment.
Ok, I crushed those commits and put the regex back. I tried to keep the Regex as short as possible, so I no went for
!/Z$/i.test(date) rather then /[^Z]$i.test(date)`, as it is
- less bytes (not many)
- easier to comprehend the negation outside of the regex instead of with the [^Z] negation syntax. Is that ok now?
| @@ -46,6 +46,40 @@ describe('Parse', () => { | |||
| expect(dayjs(time).valueOf()).toBe(moment(time).valueOf()) | |||
| }) | |||
|
|
|||
| it('String RFC 2822, time and zone', () => { | |||
There was a problem hiding this comment.
The added test suit here seems just test Date object rather dayjs here?
There was a problem hiding this comment.
I just wanted to make that functionality explicit, because right now it is neither documented nor explicitly handled in the tests. Whenever we change our internal implementation we now get feedback whether our assumptions are still valid, right? Will tell you about breaking changes then.
ghost
mentioned this pull request
suspectpart
force-pushed
the
486-refactor-parse-date
branch
from
c6134e6 to
4fa7ddf
Compare
suspectpart
force-pushed
the
486-refactor-parse-date
branch
from
a106c24 to
6669985
Compare
|
Are we good to merge this and focus on the other PR (shortening the regex)? The other one is ready to be checked with the CI build and cross-browser testing. |
|
🎉 This PR is included in version 1.8.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
See #486