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

Duration::round incorrectly bubbling time units for DST gaps #2816

Closed
arshaw opened this issue Apr 4, 2024 · 3 comments
Closed

Duration::round incorrectly bubbling time units for DST gaps #2816

arshaw opened this issue Apr 4, 2024 · 3 comments

Comments

@arshaw
Copy link
Contributor

arshaw commented Apr 4, 2024

This has a distinct root cause from #2742, not fixed by #2810.

Repro:

dur = Temporal.Duration.from({ months: -1, hours: -24 })
dur.round({
  smallestUnit: 'millisecond',
  relativeTo: '2024-04-11T02:00:00[America/New_York]'
  // fyi, 2024-03-10T02:00:00 is a spring-forward DST gap
}).toString()

// EXPECTED: -P1MT23H
// ACTUAL: -P1MT1H (looses almost a whole day!)

// The "EXPECTED" is correct because:
Temporal.ZonedDateTime.from('2024-04-11T02:00:00[America/New_York]')
  .until('2024-03-10T02:00:00[America/New_York]', { largestUnit: 'months' })
  .toString() // -P1MT23H

This bug also exists in fullcalendar's temporal-polyfill but for different reasons.

@ptomato
Copy link
Collaborator

ptomato commented May 2, 2024

With the fullcalendar polyfill as well as the new algorithm in #2758 I get the same results:

const dur = Temporal.Duration.from({ months: -1, hours: -24 });
const relativeTo = Temporal.ZonedDateTime.from('2024-04-11T02:00:00[America/New_York]');
const result = dur.round({
  smallestUnit: 'millisecond',
  relativeTo
});
console.log(result.toString());  // -P1M1DT1H
const end = relativeTo.add(dur);
console.log(end.toString());  // 2024-03-10T01:00:00-05:00[America/New_York]
console.log(relativeTo.until(end, { largestUnit: 'months' }).toString());  // -P1M1DT1H

So the original bug seems like it is fixed, but the result is still different from the expectation above. At first glance it seems like -P1MT23H might be wrong? Subtracting 1 month from 2024-04-11T02 gives 2024-03-11T02, and subtracting another 23 hours from that gives either 2024-03-10T03 or 2024-03-10T02; but adding the original duration gives 2024-03-10T01.

@arshaw
Copy link
Contributor Author

arshaw commented May 16, 2024

I was wrong about what the EXPECTED case should be. Corrected:

dur = Temporal.Duration.from({ months: -1, hours: -24 })
relativeTo = Temporal.ZonedDateTime.from('2024-04-11T02:00:00[America/New_York]')
// fyi, 2024-03-10T02:00:00 is a spring-forward DST gap
//
dur.round({ smallestUnit: 'millisecond', relativeTo }).toString()
//
// EXPECTED: -P1M1DT1H
// ACTUAL: -P1MT1H (looses a whole day!)

// The "EXPECTED" is correct because:
//
d2 = relativeTo.add({ months: -1, hours: -24 }) // '2024-03-10T01:00:00-05:00[America/New_York]'
//
relativeTo
  .until(d2, { largestUnit: 'months' })
  .toString() // -P1M1DT1H

So, the original bug has been fixed, and the correct result is represented in the main branch of this repo as well as fullcalendar's polyfill.

@ptomato
Copy link
Collaborator

ptomato commented May 17, 2024

Yep, I agree with your reasoning. Glad this is fixed!

@ptomato ptomato closed this as completed May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants