js-temporal/temporal-polyfill

inconsistent `since` calcualtions with `ZonedDateTime`

khawarizmus opened this issue · 5 comments

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?

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

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

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)

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.

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