moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.41k stars 729 forks source link

Wrong diff (when `future_date.diff(past_date)` #1200

Open nitin2953 opened 2 years ago

nitin2953 commented 2 years ago

Describe the bug

Usually we compare from past_date to future_date

But in below example when comparing from future_date to past_date, it is showing wrong diff till 25 DAYS after every 35/36 DAYS until the past_date surpasses future_date.

luxonDiff("2022-07-05", "2022-03-28")
// {years: 0, months: -3, days: -7} ❌

luxonDiff("2022-07-05", "2022-03-29")
// {years: 0, months: -3, days: -6} ❌

luxonDiff("2022-07-05", "2022-03-30")
// {years: 0, months: -3, days: -5} ❌

luxonDiff("2022-07-05", "2022-03-31")
// {years: 0, months: -3, days: -5} ✅

luxonDiff("2022-07-05", "2022-04-01")
// {years: 0, months: -3, days: -4} ✅

luxonDiff("2022-07-05", "2022-04-02")
// {years: 0, months: -3, days: -3} ✅

To Reproduce

function luxonDiff(date1, date2) {
    let end = luxon.DateTime.fromISO(date1);
    let start = luxon.DateTime.fromISO(date2);
    let diff = start.diff(end, ['years', 'months', 'days']);
    return diff.toObject();
}

Actual vs Expected behavior I'm comparing Luxon with Temporal API (polyfills), which is showing correct result

Temporal Example ```js function TemporalDiff(date1, date2) { d1 = Temporal.PlainDate.from(date1).withCalendar('iso8601'); d2 = Temporal.PlainDate.from(date2).withCalendar('iso8601'); diff = d1.until(d2, { largestUnit: 'year' }); return diff } TemporalDiff("2022-07-05", "2022-03-28") // {years: 0, months: -3, days: -8 } ✅ TemporalDiff("2022-07-05", "2022-03-29") // {years: 0, months: -3, days: -7 } ✅ TemporalDiff("2022-07-05", "2022-03-30") // {years: 0, months: -3, days: -6 } ✅ TemporalDiff("2022-07-05", "2022-03-31") // {years: 0, months: -3, days: -5 } ✅ TemporalDiff("2022-07-05", "2022-04-01") // {years: 0, months: -3, days: -4 } ✅ TemporalDiff("2022-07-05", "2022-04-02") // {years: 0, months: -3, days: -3 } ✅ ```

Desktop (please complete the following information):

Additional context ---

icambron commented 2 years ago

The problem here is how Luxon handles the end of the month. What's 03-31 + 1 month and how should it be different than 03-30 + 1 month? The way Luxon does that, they both result in April 30. The diff figures out how many months it can add to 03-30 before getting to 07-05 and then figures out how many days. So there's a day in there getting "absorbed" by the fact that you can't add months cleanly.

This isn't a bug; it's ambiguity in the concepts themselves. Here's a fuller explanation: https://github.com/moment/luxon/issues/654#issuecomment-585053355

nitin2953 commented 2 years ago

This isn't a bug

But the output is wrong & end user ONLY wants perfect result.

luxonDiff("2022-07-05", "2022-03-30")
// {years: 0, months: -3, days: -5} ❌

luxonDiff("2022-07-05", "2022-03-31")
// {years: 0, months: -3, days: -5} ✅

What do I do now?

nitin2953 commented 2 years ago

Another one

luxonDiff("2022-01-28", "2021-11-27");
// {years: 0, months: -2 days: -1} ✅

luxonDiff("2022-01-28", "2021-11-28");
// {years: 0, months: -2 days: 0} ✅

luxonDiff("2022-01-28", "2021-11-29");
// {years: 0, months: -1 days: -30} ❌ SHOULD BE 29

luxonDiff("2022-01-28", "2021-11-30");
// {years: 0, months: -1 days: -29} ❌ WHERE IS 28

luxonDiff("2022-01-28", "2021-12-01");
// {years: 0, months: -1 days: -27} ✅

luxonDiff("2022-01-28", "2021-12-02");
// {years: 0, months: -1 days: -26} ✅
sam-s4s commented 2 years ago

Can I make an observation? I think you're both right, but I have an idea that might work...

Due to the incompatible nature between months and days, we have to be careful when we cross the boundary between the groups.

To get an intuitive answer that a human would expect, we always need to use the upper day limit of the lower date. The higher of the 2 dates doesn't matter at all, because its lower boundary is always 1.

For example, if the lower date is "2022-03-30" then the upper boundary is 31. If the lower date is "2022-04-30" then the upper boundary is 30.

So, short answer, always use the upper day limit of the lower date.