luxon

luxon copied to clipboard

Describe the bug If we have a span of exactly one year, shiftTo adds several days even though it should not. shiftToAll adds one month.

This worked fine in version 3.3.0 and seems to be broken since 3.4.0.

To Reproduce Please share a minimal code example that triggers the problem:

    const from = DateTime.fromObject({
      "year": 2023,
      "month": 5,
      "day": 31,
      "hour": 0,
      "minute": 0,
      "second": 0,
      "millisecond": 0
    });
    const to = DateTime.fromObject({
      "year": 2024,
      "month": 5,
      "day": 30,
      "hour": 0,
      "minute": 0,
      "second": 0,
      "millisecond": 0
    });

Actual vs Expected behavior

// returns { "years": 1,   "months": 0,    "days": 5,    "hours": 0,    "minutes": 0,    "seconds": 0,    "milliseconds": 0}
// expected  { "years": 1,   "months": 0,    "days": 0,    "hours": 0,    "minutes": 0,    "seconds": 0,    "milliseconds": 0}
console.log(to.diff(from).shiftTo('years', 'months', 'days', 'hours', 'minutes', 'seconds', 'milliseconds').toObject());

// returns {    "years": 1,    "months": 1,    "weeks": 0,    "days": 1,    "hours": 0,    "minutes": 0,    "seconds": 0,    "milliseconds": 0}
// expected  {    "years": 1,    "months": 0,    "weeks": 0,    "days": 0,    "hours": 0,    "minutes": 0,    "seconds": 0,    "milliseconds": 0}
console.log(to.diff(from).shiftToAll().toObject());

// returns {    "days": 365 } <-- which is ok
console.log(to.diff(from).shiftTo('days').toObject());

Desktop (please complete the following information):

• OS: Windows
• Browser [e.g. Chrome 84, safari 14.0]: Chrome 124
• Luxon version [e.g. 1.25.0]: since 3.4.0
• Your timezone [e.g. "America/New_York"]: Germany/Berlin

Unfortunately I do not think this is solvable. A Duration has no concept of the calendar and as such, when you convert between calendar units (months, years), things can be ambigous.

Example: Duration.fromObject({ months: 1, days: 36 }).normalize()

This can be anything from 2 months and 6 days (which it will be, because Duration assumes a month is 30 days), but it might also be 2 months and 8 days, if the month you're talking about would be February. But Duration has no way of knowing this. This is precisely why DateTime#diff takes a set of units, because it does have the context of the actual dates, it knows which months you're talking about. Therefor it can give you the correct answer. But as soon as it returns, that information is lost and not preserved in Duration.

In your first example, you are taking the diff in milliseconds (the default). This is 31536000000ms. Then you're asking Luxon to convert this into all units. Now it depends on if you start filling up "from the bottom" or "from the top" which result you get. In this case Luxon fills up the units "from the bottom". The milliseconds go into seconds, then minutes, etc. In this case you eventually end up with 365 days and then 52 weeks and 1 day - so far everything is unambiguous. But you can already tell that things have gone awry, because suddenly we have "an extra day" (because don't years have 52 weeks exactly? Well, no they do not).

Luxon 3.3.0 had a different algorithm for Duration#normalize so might have given you a different result here, but both results are correct in terms of the casual conversion matrix.

But it looks really weird:

const yearInMs = Duration.fromObject({ year: 1 }).toMillis();

Duration.fromMillis(yearInMs).rescale().toHuman(); // => 1 year, 1 month, 1 day

IMO the correct logic should be the following:

• convert all units to milliseconds (totalMs)
• add them up
• sort all requested units in the order year => millisecond
• assign restMs = totalMs
• in the loop through these sorted units divide restMs by the number of milliseconds in current unit e.g. 1000 for second, 60 * 1000 for minute etc. and assign the reminder of division to restMs

And looks like this logic was used in Luxon <= 3.4.1 as those versions return correct results.

Unfortunately your algorithm falls apart very quickly. Take this example: Duration.fromObject({ years: 1 }).shiftTo('months') and let's run through your steps:

1. convert all units to milliseconds: totalMs = 31536000000 (1 * (365*24*60*60*1000))
2. add them up: totalMs = 31536000000 (trivial)
3. Sort all requested units in the order: orderedUnits = ['months']
4. assign restMs = 31536000000
5. Loop:

• currentUnit = months
• converted = 31536000000 / 2592000000 (1 month has 30*24*60*60*1000 = 2592000000 milliseconds)
• converted = 12.166666667

6. Result: 1 year has 12.1666667 months.

Result: 1 year has 12.1666667 months.

Yep, and that's exactly what previous versions of Luxon resulted with.

That's ok as we treat one month to be ~30 days and it's explained in details in Luxon docs.

The code you shared is not the same conversation that I posted about. You shifted milliseconds to months, not years to months.

I am talking about Duration.fromObject({ years: 1 }).shiftTo('months').toObject(). This has always produced { months: 12 } and your algorithm does not do this.

Ah, ok, sorry. But what was the problem in the previous algorithm?

Mostly negative numbers: https://github.com/moment/luxon/issues/1233