fix: diff(year/month/quarter) returns NaN instead of 0 for Invalid Date - #3187
fix: diff(year/month/quarter) returns NaN instead of 0 for Invalid Date#3187koding88 wants to merge 3 commits into
Conversation
prettyUnit consulted the special map before lowercasing, then
stripped a trailing 's', so 'MS'/'Ms'/'mS' resolved to the string
'm'. Consequences while every other unit was case-insensitive:
- get('MS') threw TypeError (this.m is not a function)
- set('MS', n) was a silent no-op
- startOf/endOf('MS') were clone-only no-ops
Look up the special map again after lowercasing so mixed-case ms
behaves exactly like 'ms'.
monthDiff had `|| 0` which converted NaN to 0 when either operand was an Invalid Date. Replaced with `+ 0` to normalize -0 to +0 while letting NaN propagate, matching the behavior of all other diff units. Fixes iamkun#3186
There was a problem hiding this comment.
🟡 Changes recommended
The new Millisecond test compares two separate dayjs() instances and can be time-dependent/flaky; it should be made deterministic before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Day.js .diff() behavior for year/month/quarter units so that comparisons involving an Invalid Date propagate NaN (instead of being silently coerced to 0) while still normalizing -0 to +0 for valid calculations.
Changes:
- Updated
monthDiffto preserveNaNresults by removing the|| 0fallback and normalizing-0via+ 0. - Expanded
prettyUnitto recognize mixed-case millisecond unit inputs (e.g.,MS,Ms,mS) and added related tests. - Added regression tests for issue #3186 covering invalid-date operand ordering and a moment.js comparison for valid dates.
File summaries
| File | Description |
|---|---|
| src/utils.js | Fixes monthDiff NaN propagation and improves unit normalization in prettyUnit. |
| test/issues/issue3186.test.js | Adds coverage for NaN propagation in diff() for year/month/quarter with invalid dates and a valid-date regression check. |
| test/utils.test.js | Extends prettyUnit tests to validate mixed-case millisecond unit inputs. |
| test/get-set.test.js | Adds get/set/startOf tests for mixed-case millisecond unit inputs. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expect(dayjs().millisecond()).toBe(moment().millisecond()) | ||
| expect(dayjs().millisecond(0).valueOf()).toBe(moment().millisecond(0).valueOf()) | ||
| expect(dayjs().millisecond(1).valueOf()).toBe(moment().millisecond(1).valueOf()) | ||
| expect(dayjs().get('MS')).toBe(dayjs().get('ms')) |
nrps9909
left a comment
nrps9909
left a comment
There was a problem hiding this comment.
Reviewed 07c5726c8a0ac211af5744e9ccaf743a70289af0 against base 0f6c19e3b63bcc3ff74917cb3a60125020c75648. The monthDiff NaN change looks correct: across UTC, New York and Kathmandu, 216 invalid-date observations fail on base and none fail on head (648 observations total). Another 3,600 comparisons of valid dates against Moment agree, including month ends, leap dates, local/UTC instances, floating/integer results and zero sign. Full npm test passes all eight timezone legs and 94 suites / 798 tests, meeting the 100% line-coverage gate; lint passes. The previously flagged two-instance millisecond assertion is already fixed on this head.
One scope question before approving the whole diff: prettyUnit also adds special[lower], independently of the NaN fix. This changes more than mixed-case ms. With a fixed d = dayjs('2024-06-15T12:34:56.789'):
| Expression | Base | This head |
|---|---|---|
d.add(1, 'H').valueOf() - d.valueOf() |
1 ms | 3,600,000 ms |
d.add(1, 'S').valueOf() - d.valueOf() |
1 ms | 1,000 ms |
d.add(1, 'W').valueOf() - d.valueOf() |
1 ms | 604,800,000 ms |
These were unsupported spellings taking the existing fallback, rather than documented valid inputs regressing. However, the Add documentation explicitly treats short forms as case-sensitive. Could the unit-normalization change and its get/set tests be separated from this invalid-date fix, or could maintainers confirm the intended API expansion and corresponding documentation/types? The invalid-date correction can stand on its own without that decision.
No upstream status checks were exposed for this head. This is a scoped validation comment, not an approval of the unrelated unit-contract change. Review and local validation performed with Codex.
Fixes #3186.
monthDiffinsrc/utils.jsended with|| 0, which converted theNaNproduced by Invalid Date operands into a plausible-looking0. As a result,.diff(other, 'year'|'month'|'quarter', true)silently reported "0 units apart" for a meaningless comparison, while every other unit (day,hour,minute,second,week) correctly returnedNaNfrom the raw millisecond difference.Replaced
|| 0with+ 0to normalize-0to+0(the original purpose of the fallback) while lettingNaNpropagate.Added test covering year/month/quarter for both operand orderings, plus a regression check against moment.js for valid dates. All 798 tests pass, lint clean.