Refactor (and redesign?) parseDate()
- Dominant language
- JavaScript
- Stars
- 48.7k
- Forks
- 2.5k
- PR merge metrics
- No merged PRs in 30d
Description
@iamkun Here is the issue with my comments, following the discussion in #417.
https://github.com/iamkun/dayjs/blob/a7e05e0b7fa529e8d81ebec3f007eb817d970fdd/src/index.js#L53-L61
Here I want to get rid of the warning `eslint-disable-next-line no-cond-assign`, I think that is not good style. Also, the check whether there is a `z` in the end of the string or not does not need to be a regex, which is also hinted by the comment `looking for a better way`. I propose these changes:
```javascript
if (typeof date === 'string' && !date.toLowerCase().endsWith('z')) {
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)
}
}
```
this also gets rid of the `let reg` in the first line of `parseDate`, moving the variable down where it is needed, renaming it to just `d` as `reg` was also quite inprecise as this is not a regex but rather matched parts of a date.
https://github.com/iamkun/dayjs/blob/a7e05e0b7fa529e8d81ebec3f007eb817d970fdd/src/index.js#L63
Here the comment is misleading. This is **not** the default return for timestamps only, but all of the following inputs will end up in this last return statement:
- `parseDate({}) // objects` (invalid)
- `parseDate(function() {}) // function` (invalid)
- `parseDate(Infinity) // non-finite number` (invalid)
- `parseDate(12345) // finite number` (valid, interpreted as unix timestamp)
- `parseDate('Mon Jan 01 2018 00:00:00 GMT+0100 (Mitteleuropäische Normalzeit)') // RFC 2822 date string` (valid)
- `parseDate(() => '2018-01-01') // arrow func` (valid in Chrome, invalid in Firefox)
- `parseDate([2018, 1, 1]) // Array` (valid in both Firefox and Chrome!)
So I got two issues with this:
- The comment is misleading and most of the cases I mention above are not explicitly tested by the dayjs test suite
- We do not abstract the Date constructor default behavior, even causing incosistencies between different browsers
In my opinion, the second point is what I expect dayjs to do - provide a clear and straightforward API to create dates without any weird special cases. Instead almost everything is forwarded to the Date constructor, and adding more complexity even:
- `new Date(null)` is fine in standard Javascript (and equivalent to `new Date(0)`, so 1970-01-01T00:00:00.000, but `null` is **not** allowed with dayjs
- dayjs also allows for all the moment.js special strings like `20180313` or `20180313 12:24:33`
- ... but in other cases dayjs is **not** compatible with moment.js (e.g. `moment({})` is now, where `dayjs({})` is invalid)
- most of the special cases (RFC2822 strings, momentjs-like-strings etc.) are not documented in the [API documentation](https://github.com/iamkun/dayjs/blob/dev/docs/en/API-reference.md#constructor-dayjsexisting-string--number--date--dayjs)
So what is the overall design goal here? Should it be a 100% compatible with moment.js? Or should it abstract from weird browser behavior? Both is not given, and it might be confusing to the user.
Contributor guide
Assessment
This issue has not been assessed yet.