js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
522 stars 28 forks source link

inconsistent `since` calcualtions with `ZonedDateTime` #281

Closed khawarizmus closed 6 months ago

khawarizmus commented 6 months ago

I am facing a wired bug with the current polyfill and here is an example to illustrate the problem:

const pivotPlainDate = Temporal.PlainDate.from('2020-08-20')
const yearPlainStart = pivotPlainDate.with({ month: 1, day: 1 })

const pivotPlainDateTime = pivotPlainDate.toPlainDateTime({ hour: 12 })
const yearPlainDateTimeStart = pivotPlainDateTime.with({ month: 1, day: 1 })

const pivotZonedDateTime = pivotPlainDateTime.toZonedDateTime('UTC')
const yearZonedDateTimeStart = pivotZonedDateTime.with({ month: 1, day: 1 })

console.log('Weeks since year start [PlainDate]', Math.ceil((pivotPlainDate.since(yearPlainStart).days + 1) / 7)) // 34
console.log('Weeks since year start [PlainDateTime]', Math.ceil((pivotPlainDateTime.since(yearPlainDateTimeStart).days + 1) / 7)) // 34
console.log('Weeks since year start [ZonedDateTime]', Math.ceil((pivotZonedDateTime.since(yearZonedDateTimeStart).days + 1) / 7)) // 1 => incorrect

As it can be seen in the example above (pivotZonedDateTime.since(yearZonedDateTimeStart).days + 1) / 7) is yielding 1 instead of 34 and this happens only for the ZonedDateTime Temporal Object. Is this an expected behaviour? am I missing something here?

khawarizmus commented 6 months ago

I have solved this by adding { largestUnit: 'days' } as such

console.log('Weeks since year start [ZonedDateTime]', Math.ceil((pivotZonedDateTime.since(yearZonedDateTimeStart, { largestUnit: 'days' }).days + 1) / 7)) // -> 34

But i would appreciate to know if this is an expected behaviour

ptomato commented 6 months ago

This is expected. The default largestUnit for ZonedDateTime since/until is hours: https://tc39.es/proposal-temporal/docs/zoneddatetime.html#until

That's because days may be different lengths in ZonedDateTime calculations. Hours are the largest unambiguous unit, so that's why they're the default. In PlainDate / PlainDateTime calculations, days are the largest unambiguous unit, so that's why they are the default there.

You can calculate directly with weeks, though. Here's how I'd recommend doing it:

const options = { smallestUnit: 'weeks', roundingMode: 'ceil' };
console.log('Weeks since year start [PlainDate]',
  yearPlainStart.until(pivotPlainDate, options).weeks); // 34
console.log('Weeks since year start [PlainDateTime]',
  yearPlainDateTimeStart.until(pivotPlainDateTime, options).weeks); // 34
console.log('Weeks since year start [ZonedDateTime]',
  yearZonedDateTimeStart.until(pivotZonedDateTime, options).weeks); // 34
khawarizmus commented 6 months ago

Thank you @ptomato for the detailed explanation. It makes sense to me. I was actually worried to use weeks as i wasn't sure what definition of week we are talking about. In ISO a week is always 7 days wheres a calendar week can be shorter then that.

Is the week here always considered 7 days? and would there be edge cases (for example year boundaries) where this calculation might fail?

For context this code is used to calculate week dates which i published yesterday https://week-dates.netlify.app (still polishing the docs)

ptomato commented 6 months ago

Depends on the calendar. For PlainDate/PlainDateTime/ZonedDateTime instances in the ISO calendar, the week is always 7 days. If you want to ensure that, use .withCalendar("iso8601") before doing the calculation.

If you want the behaviour where the calendar determines the length of a week, no need to do anything. (Although I don't know the status of how accurate the CLDR calendars' week information is.)

There should not be edge cases around year boundaries in the ISO calendar.

khawarizmus commented 6 months ago

This is very clear to me thank you. In my usecase days makes more sense since I am dealing with calendars other than iso8601 and scales other then Gregorian