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

moment().diff(moment()) does not always return 0 #5195

Closed
jo-flynn opened this issue Aug 13, 2019 · 6 comments · Fixed by moment/momentjs.com#686
Closed

moment().diff(moment()) does not always return 0 #5195

jo-flynn opened this issue Aug 13, 2019 · 6 comments · Fixed by moment/momentjs.com#686

Comments

@jo-flynn
Copy link

jo-flynn commented Aug 13, 2019

Describe the bug
When repeatedly running this calculation: moment().add(1, 'years').diff(moment(), 'days') when the current date is in a year before a leap year (in my case, 2019/08/13), moment inconsistently returns 365 or 366. Adding .startOf('day') to each moment() will consistently return 366.

To Reproduce
Steps to reproduce the behavior:

  1. Execute moment().add(1, 'years').diff(moment(), 'days') at least 4 times.
  2. Compare outputs, you'll see 365 and 366 inconsistently.
  3. Execute moment().startOf('day').add(1, 'years').diff(moment().startOf('day'), 'days') at least 4 times.
  4. Compare outputs, you should see 366 each time.

Expected behavior
moment should consistently account for leap years when calculating day differences.

Desktop (please complete the following information):

  • OS: Windows & MacOS
  • Browser: Chrome
  • Version 76

Moment-specific environment

  • The time zone setting of the machine the code is running on: PST
  • The time and date at which the code was run: See below
  • Other libraries in use (TypeScript, Immutable.js, etc): Bug occurred in a CRA React app, but is replicable on https://momentjs.com in the console.

Please run the following code in your environment and include the output:

console.log((new Date()).toString())
console.log((new Date()).toLocaleString())
console.log((new Date()).getTimezoneOffset())
console.log(navigator.userAgent)
console.log(moment.version)

Output:

Tue Aug 13 2019 12:30:35 GMT-0700 (Pacific Daylight Time)
8/13/2019, 12:30:35 PM
420
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.100 Safari/537.36
2.24.0
@ashsearle
Copy link
Contributor

ashsearle commented Aug 14, 2019

The inconsistency is due to the two moment() instances sometimes representing different points in time.

i.e. moment().diff(moment()) does not always return 0 - it depends if the instances are created in the same millisecond.

In your case, the moment() on the left may be created a fraction of a second before the moment() on the right; which means the difference between the two instances is slightly less than 366 days. Add true as a final argument to get fractional days:

// this might return exactly 366, or something like 365.9999999768518
moment().add(1, 'years').diff(moment(), 'days', true);

@jo-flynn
Copy link
Author

jo-flynn commented Aug 20, 2019

@ashsearle Would it be appropriate to document this behaviour? I spent the better part of a day debugging this as it wasn't apparent (to me, anyway) that two moment()s created in the same line of code could represent different points in time, and I can imagine other folks running into this.

@ashsearle
Copy link
Contributor

ashsearle commented Aug 21, 2019

I'm not sure.

I know you're not the only person to get tripped up by this, but it's no different to being surprised that [Date.now(), Date.now()] may contain two different points in time.

Let's suppose it was documented: did you look for the documentation? Where did you look?

@jo-flynn
Copy link
Author

jo-flynn commented Aug 21, 2019

I read the docs for Now, as well as Difference. In my opinion, it would make sense to note that initializing multiple moments in the same line of code won't guarantee identical moment instances as part of the Now section (as the behaviour is initialization-related.)

As I've been a JS dev for about 10 years and this behaviour surprised me, I think it's reasonable to assume this isn't common knowledge? Part of the appeal of moment is that as a user, I don't need deep knowledge of all the quirks involved in working with Dates to build functional & reliable code, so pointing out stuff like this seems in line with that philosophy?

If you agree that this should be documented, I can work on a PR soon. I'm obviously open to feedback on where it should go 🙂

@ashsearle
Copy link
Contributor

ashsearle commented Aug 22, 2019

I agree the Now section feels like the right place to put it.

Let's talk about the example in that section:

var x = undefined;
moment(x).isSame(moment(), 'second'); // true

I think it's written that way to avoid implying moment(undefined).isSame(moment()) - with the 'second' qualifier attempting to provide fault tolerance for the two moment's representing fractionally different points in time.

However that's a bad way to do things as it's occasionally going to act like:

var a = moment();
var b = a.clone().add(1, 'millisecond');
a.isSame(b, 'second'); // false if ms wrapped rolled over from 999 to 0

It would probably be better to change that example to a diff and add an explicit comment about it.

@marwahaha marwahaha changed the title Inconsistent output from .diff(moment(), 'days') if upcoming year is leap year moment().diff(moment()) does not always return 0 May 18, 2020
@SergeySerj
Copy link

SergeySerj commented Jun 17, 2020

Agree with @ashsearle!

Otherwise the below code returns random results depending on current time.

var now = moment.utc();
var a = now.clone().add(30, 'second');
var b = now.clone().subtract(30, 'second');
a.isSame(now, 'minute');
b.isSame(now, 'minute');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants