.add() edge case failure

[JohanRonnblom]

Description of the Issue and Steps to Reproduce:

Adding something just less than a month produces unexpected results:

var start = moment('2017-03-01');
var dur = moment.duration(1, 'month').subtract(1, 'second');
start.add(dur).toDate();

Tue Mar 28 2017 23:59:59 GMT+0200 (W. Europe Daylight Time)

Expected:

Fri Mar 31 2017 23:59:59 GMT+0200 (W. Europe Daylight Time)

Cause: moment apparently subtracts milliseconds first, then presumably uses daysInMonth() on the intermediate object to subtract 28 days for February. Rather, it should use daysInMonth() on the original object.

---

Reporting system info (probably completely irrelevant):

console.log( (new Date()).toString())

Wed Jun 07 2017 14:16:24 GMT+0200 (W. Europe Daylight Time)
console.log((new Date()).toLocaleString())
6/7/2017, 2:16:47 PM
console.log( (new Date()).getTimezoneOffset())
-120
console.log( navigator.userAgent)
Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.98 Safari/537.36
console.log(moment.version)
2.18.1

[maggiepint]

Durations are fixed numbers of milliseconds. A duration that is 1 month long is exactly the number of milliseconds in 30 calendar days. The intended use case for durations is around measuring... well durations. Lengths of time that are not related to dates.

I think the code you are looking for might be code for calendar computations, where the amount of days in a month changes based on the month in question.

Does this look right?

moment('2017-03-01').add(1, 'month').subtract(1, 'millisecond').format()
"2017-03-31T23:59:59-07:00"

[JohanRonnblom]

A duration that is 1 month long is exactly the number of milliseconds in 30 calendar days.

This statement is not correct. As you can see in your example, 31 days are added, not 30. And in my example, 28 days are added, again not 30.

When adding a duration to a moment, units such as months and years must be interpreted in a context, since they have a different duration depending on whether a year is a leap year and due to different numbers of days in months. In my opinion, the only sensible context is the context of the moment to which the duration is added. Possibly this can be achieved by simply changing the order to first handle years, then months, then shorter units.

[JohanRonnblom]

add-subtract.js performs the calculation for milliseconds, days and months, in that order. Other periods such as years, weeks, hours etc are stored in a duration as their closest shorter counterparts.

Is there another use case where reversing this order of operation would produce undesired results? I can't come up with such a situation.

[icambron]

Yes, duration addition with higher-order units uses calendar math, not timestamp math. I think I agree that the current behavior is incorrect wrt order of operations. A more common case:

moment('2016-02-28').add(moment.duration(1, 'month').add(3, 'days'))

//should that behave like a)

moment('2016-02-28').add(1, 'month').add(3, 'days') // => "2016-03-31T00:00:00-04:00"

//or like b)

moment('2016-02-28').add(3, 'days').add(1, 'month') // => "2016-04-02T00:00:00-04:00"

b is the current behavior and seems much less intuitive than a.

set doesn't analogize perfectly, but it seems like a good hint that we do the sets in little endian order for similar reasons.

[JohanRonnblom]

For what it is worth, I have checked that changing the order does not break any tests. However I can't push my changes.

[ashsearle]

This looks like a problem with duration subtract:

moment.duration(1, 'day').add(1, 'hour').toString(); // 'P1DT1H'
moment.duration(1, 'day').subtract(1, 'hour').toString(); // 'P1DT1H' - the same!

[icambron]

@ashsearle I think that's a different bug, having to do with serializing negative duration components to string, not with the mechanics of add and subtract itself.

[icambron]

@JohanRonnblom Why can't you push your changes? A pull request would be great here