Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTC Plugin calculates incorrect results when using diff() in months (starting in v1.8.33) #1021

Closed
KosnIre opened this issue Aug 21, 2020 · 8 comments · Fixed by #1171
Closed
Labels

Comments

@KosnIre
Copy link

KosnIre commented Aug 21, 2020

Describe the bug
Using the diff() method, in months, returns incorrect results when using the UTC plugin starting in v1.8.33. It is also returning different results when using the plugin vs without.

The result appears to be off by one month in some cases.

NOTE: At first I thought it was related to #1000 but I think this is a different issue. For one, it affects a different version (1.8.33 here vs 1.8.32 there) and also that issue does not reference the UTC plugin. I also note that there were a few UTC related changes in 1.8.33.

Expected result: The difference between May and December should be calculated as 7 months.

Actual result: The difference between May and December is calculated as 6 months.

Information

  • Affected Day.js Versions: 1.8.33, 1.8.34
  • Unaffected Day.js Versions: 1.8.32 and below
  • OS: Ubuntu 18.04
  • Node v12.18.2
  • Time zone: GMT-04:00 EDT (Eastern Time)
@KosnIre
Copy link
Author

KosnIre commented Aug 21, 2020

Test code

const dayjs = require('dayjs');
const utc = require('dayjs/plugin/utc');

dayjs.extend(utc);


/**
 * calcDateDiff - Test method that finds the difference between 2 ISO dates using dayjs.
 */ 
function calcDateDiff({ startDateISO, endDateISO, timeUnits }, utc=false) {
    let startDate, endDate;
    
    if (utc) {
        startDate = dayjs.utc(startDateISO); // convert to dayjs
        endDate = dayjs.utc(endDateISO);
    } else {
        startDate = dayjs(startDateISO); // convert to dayjs
        endDate = dayjs(endDateISO);
    }

    const dateDiff = endDate.diff(startDate, timeUnits);
    
    return dateDiff;
}

// Create test data

const info = {
    endDateISO: '2019-12-15T11:00:00',
    timeUnits: 'month',
};

const data = [];
let startDateISO;

// Add 12 start dates to our test data, going further into the past so we can compare results. 
for (let i=12; i >=1; i--) {
    const leadingZero = i < 10 ? '0' : '';
    startDateISO = `2019-${leadingZero}${i}-15T11:00:00`;
    data.push({
        ...info,
        startDateISO,
    });
}

// Run the test and ouput results to the console
for (const d of data) {
    const dateDiff = calcDateDiff(d);
    const dateDiffUTC = calcDateDiff(d, true);
    
    console.log('Start date:', d.startDateISO, 'End date:', d.endDateISO, 'diff', dateDiff, 'diff-UTC', dateDiffUTC);
}

Test results
Test results shown from v1.8.33. Note the series labeled "diff" (0, 1, 2, 3) vs the series labeled "diff-UTC" (0, 1, 1, 2). Oddly the sequences re-align at 10 and 11.

Start date: 2019-12-15T11:00:00 End date: 2019-12-15T11:00:00 diff 0 diff-UTC 0
Start date: 2019-11-15T11:00:00 End date: 2019-12-15T11:00:00 diff 1 diff-UTC 1
Start date: 2019-10-15T11:00:00 End date: 2019-12-15T11:00:00 diff 2 diff-UTC 1
Start date: 2019-09-15T11:00:00 End date: 2019-12-15T11:00:00 diff 3 diff-UTC 2
Start date: 2019-08-15T11:00:00 End date: 2019-12-15T11:00:00 diff 4 diff-UTC 3
Start date: 2019-07-15T11:00:00 End date: 2019-12-15T11:00:00 diff 5 diff-UTC 4
Start date: 2019-06-15T11:00:00 End date: 2019-12-15T11:00:00 diff 6 diff-UTC 5
Start date: 2019-05-15T11:00:00 End date: 2019-12-15T11:00:00 diff 7 diff-UTC 6
Start date: 2019-04-15T11:00:00 End date: 2019-12-15T11:00:00 diff 8 diff-UTC 7
Start date: 2019-03-15T11:00:00 End date: 2019-12-15T11:00:00 diff 9 diff-UTC 8
Start date: 2019-02-15T11:00:00 End date: 2019-12-15T11:00:00 diff 10 diff-UTC 10
Start date: 2019-01-15T11:00:00 End date: 2019-12-15T11:00:00 diff 11 diff-UTC 11

@iamkun
Copy link
Owner

iamkun commented Aug 22, 2020

Seems ok here ? https://runkit.com/embed/7xucw8zhictx

@KosnIre
Copy link
Author

KosnIre commented Aug 23, 2020

Hmm, yes that runkit runs ok for me too. That's interesting 🤔

I see the node version is v10.22.0 on runkit, so I tried switching to that locally, but I still get incorrect results.

Would you please try running this fiddle? It gives me incorrect results in both Firefox and Chrome.

https://jsfiddle.net/1o4apxgv/

@iamkun
Copy link
Owner

iamkun commented Aug 23, 2020

Screen Shot 2020-08-23 at 1 08 51 PM

Screenshot from Chrome. Interesting 🤔

@KosnIre
Copy link
Author

KosnIre commented Aug 27, 2020

Well this is odd. Clearly there is some difference here, so I'm trying to figure out what that is.

For me, the fiddle outputs...
image

I asked a few otheres to run this test and here are some additional results for reference:

User 1: Windows, Chrome, UTC-04:00 (DST, yes). Result: Fail.
User 2: macOS, Chrome, UTC-04:00 (DST, yes). Result: Fail.
User 3: macOS, Chrome, UTC+2:00 (DST, yes). Result: Fail.
User 4: Windows, Chrome, UTC+5:30 (DST, no). Result: Pass.

My initial thought was this could have something to do with local time zone, but now I'm curious if it's related to Daylight Savings Time observance. I think I'll try to write some tests that parse the date in different timezones to see if I can find a stronger correlation.

One question: how does dayjs() internally diff() months? Is 30 days equal to one month? Is it subtracting milliseconds and converting back to months? Or is it using the native getMonth() API and subtracting the result?

@iamkun
Copy link
Owner

iamkun commented Aug 28, 2020

You can check the month diff function here

https://github.com/iamkun/dayjs/blob/dev/src/utils.js#L17

@ygj6
Copy link

ygj6 commented Sep 14, 2020

My initial thought was this could have something to do with local time zone, but now I'm curious if it's related to Daylight Savings Time observance.

@KosnIre It seems that this difference is caused by Daylight Savings Time observance.

iamkun added a commit that referenced this issue Oct 27, 2020
iamkun added a commit that referenced this issue Oct 27, 2020
iamkun added a commit that referenced this issue Oct 30, 2020
iamkun pushed a commit that referenced this issue Nov 5, 2020
## [1.9.5](v1.9.4...v1.9.5) (2020-11-05)

### Bug Fixes

* customParseFormat plugin supports parsing localizedFormats  ([#1110](#1110)) ([402b603](402b603))
* fix customParseFormat plugin parse meridiem bug ([#1169](#1169)) ([9e8f8d9](9e8f8d9)), closes [#1168](#1168)
* fix devHelper error in umd bundle in browser ([#1165](#1165)) ([d11b5ee](d11b5ee))
* fix utc plugin diff bug in DST ([#1171](#1171)) ([f8da3fe](f8da3fe)), closes [#1097](#1097) [#1021](#1021)
* isoWeek plugin type ([#1177](#1177)) ([c3d0436](c3d0436))
* update localeData plugin to support meridiem ([#1174](#1174)) ([fdb09e4](fdb09e4)), closes [#1172](#1172)
* update timezone plugin parse Date instance / timestamp logic & remove useless test ([#1183](#1183)) ([a7f858b](a7f858b))
@iamkun
Copy link
Owner

iamkun commented Nov 5, 2020

🎉 This issue has been resolved in version 1.9.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iamkun iamkun added the released label Nov 5, 2020
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this issue May 10, 2022
## [1.9.5](iamkun/dayjs@v1.9.4...v1.9.5) (2020-11-05)

### Bug Fixes

* customParseFormat plugin supports parsing localizedFormats  ([#1110](iamkun/dayjs#1110)) ([402b603](iamkun/dayjs@402b603))
* fix customParseFormat plugin parse meridiem bug ([#1169](iamkun/dayjs#1169)) ([9e8f8d9](iamkun/dayjs@9e8f8d9)), closes [#1168](iamkun/dayjs#1168)
* fix devHelper error in umd bundle in browser ([#1165](iamkun/dayjs#1165)) ([d11b5ee](iamkun/dayjs@d11b5ee))
* fix utc plugin diff bug in DST ([#1171](iamkun/dayjs#1171)) ([f8da3fe](iamkun/dayjs@f8da3fe)), closes [#1097](iamkun/dayjs#1097) [#1021](iamkun/dayjs#1021)
* isoWeek plugin type ([#1177](iamkun/dayjs#1177)) ([c3d0436](iamkun/dayjs@c3d0436))
* update localeData plugin to support meridiem ([#1174](iamkun/dayjs#1174)) ([fdb09e4](iamkun/dayjs@fdb09e4)), closes [#1172](iamkun/dayjs#1172)
* update timezone plugin parse Date instance / timestamp logic & remove useless test ([#1183](iamkun/dayjs#1183)) ([a7f858b](iamkun/dayjs@a7f858b))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this issue May 10, 2022
## [1.9.5](iamkun/dayjs@v1.9.4...v1.9.5) (2020-11-05)

### Bug Fixes

* customParseFormat plugin supports parsing localizedFormats  ([#1110](iamkun/dayjs#1110)) ([402b603](iamkun/dayjs@402b603))
* fix customParseFormat plugin parse meridiem bug ([#1169](iamkun/dayjs#1169)) ([9e8f8d9](iamkun/dayjs@9e8f8d9)), closes [#1168](iamkun/dayjs#1168)
* fix devHelper error in umd bundle in browser ([#1165](iamkun/dayjs#1165)) ([d11b5ee](iamkun/dayjs@d11b5ee))
* fix utc plugin diff bug in DST ([#1171](iamkun/dayjs#1171)) ([f8da3fe](iamkun/dayjs@f8da3fe)), closes [#1097](iamkun/dayjs#1097) [#1021](iamkun/dayjs#1021)
* isoWeek plugin type ([#1177](iamkun/dayjs#1177)) ([c3d0436](iamkun/dayjs@c3d0436))
* update localeData plugin to support meridiem ([#1174](iamkun/dayjs#1174)) ([fdb09e4](iamkun/dayjs@fdb09e4)), closes [#1172](iamkun/dayjs#1172)
* update timezone plugin parse Date instance / timestamp logic & remove useless test ([#1183](iamkun/dayjs#1183)) ([a7f858b](iamkun/dayjs@a7f858b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants