intervalToDuration fix for end of month calculations

[fturmel]

Closes #2611
Closes #1778
Closes #2470
Closes #2910

By using add internally instead of sub, the intervalToDuration function gives better results for end of month interval start dates while still passing all existing unit tests. It also means better interop with add, as discussed in #2611.

Before:

intervalToDuration({start: new Date(2021, 7, 31), end: new Date(2021, 8, 30)})
// {years: 0, months: 0, days: 30, hours: 0, minutes: 0, seconds: 0}

With this PR:

intervalToDuration({start: new Date(2021, 7, 31), end: new Date(2021, 8, 30)})
// {years: 0, months: 1, days: 0, hours: 0, minutes: 0, seconds: 0}

This also brings a few improvements:

• new RangeError thrown if interval start date is greater than end date. This is standard in all other functions handling intervals and per https://date-fns.org/docs/Interval
• additional unit tests covering new edge cases and thrown exceptions
• leaner implementation: removed dependencies to compareAsc and isValid, removed sign and Math.abs operations that were not required.

[nsafai]

i think it's worth leaving the func signature as-is, other than that this looks great, tysm for following up on this!

[nsafai]

what's the benefit of changing the func signature here? can we avoid a breaking change to syntax?

[fturmel]

Correct me if I'm wrong, but there are no signature changes here. It's not destructuring the object anymore but the only argument is still an Interval object. As I think you noticed, the change was to allow the use of start and end variable names in the function body for readability.

[nsafai]

if you really want to keep the internal variable names the same (while keeping the func signature the same as before), you could do something like this:

Suggested change

	const start = toDate(interval.start)
	const end = toDate(interval.end)
	start = toDate(start)
	end = toDate(end)

[fturmel]

Personal preference, but I don't love assignments to function arguments. I tend to treat them as read only. In this case, they are also not quite the same type. Interval start and end are Date | number but the return value of toDate is Date.

[nsafai]

lmk what you think of my suggestion of keeping func signature as-is and feel free to tag me for a +1

[fturmel]

@nsafai Thanks for taking the time and all the nice notes! As I replied in my comments I think the function signature is intact but let me know if I'm missing something.

[nsafai]

ah you're entirely right, func signature is still backwards compatible. LGTM!

[tan75]

Thank you!

[hiendaovinh]

Thanks for the fix but it's a breaking change because of the Math.abs removal. Please bump a major version next time.

[leshakoss]

@hiendaovinh sorry, that was not supposed to happen :[ A patch with a revert coming soon

[leshakoss]

@hiendaovinh actually, I don't think this is a breaking change. If interval.start was after interval.end the function would've thrown a RangeError anyway, so there's no way to get negative values from the function. If I'm wrong, please provide a test case that we can use to verify this 🙇

[fturmel]

If interval.start was after interval.end the function would've thrown a RangeError anyway

@leshakoss The RangeError was introduced in this PR, so if there are any breaking changes that would be it. However that error should've always been there if you look at the behavior of every other functions accepting an Interval in the library, per my PR notes.

[leshakoss]

Ah, I see. Honestly, this is a tough call. I would count this as a bug fix rather than a breaking change, but that's maybe because I already have "interval.start is not supposed to be after interval.end" in mind, but I understand why one might think the opposite

[hiendaovinh]

I do understand and I share the same thinking without regarding the "interval.start is not supposed to be after interval.end" stuff. Actually one of my colleges had a bug switching their places so I knew 😆. It's certainly a bug fix but also a breaking change in its own way. That' interesting.

[leshakoss]

@hiendaovinh reverted the change in v2.29.2: https://github.com/date-fns/date-fns/releases/tag/v2.29.2