@ljharb Stage 3 review

[justingrant]

This is a tracking issue for @ljharb to review the Time Zone Canonicalization proposal spec at https://tc39.es/proposal-canonical-tz/#sec-canonical-tz-intro. Jordan, feel free to add feedback via comments here or open new issues, whichever is easiest.

The spec text is easiest to review as a web page rather than the raw spec text, because most of the text is just existing AOs with only one line changed. The total scope of changes is <20 lines + one prose paragraph.

Tests can be reviewed at https://github.com/tc39/test262/pull/3837.

Thanks!

[ljharb]

In https://tc39.es/proposal-canonical-tz/#sec-temporal-timezoneequals, I'd probably skip the early returns, and change step 9 to if recordOne is not ~empty~ and recordTwo is not ~empty~ and …, but that's just a nit.

Otherwise, combined with the things mentioned in #33, LGTM!

[justingrant]

FYI: Temporal just merged an editorial PR to prepare for tc39/proposal-temporal#2607, which is the normative PR that limits offset time zones to minute precision.

As a result, today a few links in the proposal-canonical-tz spec text were broken, and a few lines of the proposal spec needed updating. I just landed a PR that catches up this proposal's spec and polyfill with those upstream changes. Apologies for the churn!

If you've already reviewed the rest of proposal-canonical-tz, please take a look at SystemTimeZoneIdentifier that has 2 changed lines + a new editor's note, and TimeZoneEquals that has 5 newly-changed lines.

AFAIK there are no more changes expected (either here or upstream) before the Bergen meeting.

[ljharb]

https://tc39.es/proposal-canonical-tz/#sec-temporal-timezoneequals step 7 looks odd; "if x is not empty or y is not empty" reads strangely to me. It seems like the "if" branch will exclude a case where both are empty. Perhaps it would be clearer to have the "if" be "if x is empty and y is empty", and then have the rest of the AO be the case where one of them isn't empty?

[justingrant]

Do you think this text would work better?

1. If _one_ and _two_ are the same Object value, return *true*.
1. Let _timeZoneOne_ be ? ToTemporalTimeZoneIdentifier(_one_).
1. Let _timeZoneTwo_ be ? ToTemporalTimeZoneIdentifier(_two_).
1. If _timeZoneOne_ is _timeZoneTwo_, return *true*.
1. <ins>Let _offsetNanosecondsOne_ be ? ParseTimeZoneIdentifier(_timeZoneOne_).[[OffsetNanoseconds]].</ins>
1. <ins>Let _offsetNanosecondsTwo_ be ? ParseTimeZoneIdentifier(_timeZoneTwo_).[[OffsetNanoseconds]].</ins>
1. <ins>If _offsetNanosecondsOne_ is ~empty~ and _offsetNanosecondsTwo_ is ~empty~, then</ins>
  1. <ins>Let _recordOne_ be GetAvailableNamedTimeZoneIdentifier(_timeZoneOne_).</ins>
  1. <ins>Let _recordTwo_ be GetAvailableNamedTimeZoneIdentifier(_timeZoneTwo_).</ins>
  1. <ins>If _recordOne_ is not ~empty~ and _recordTwo_ is not ~empty~ and _recordOne_.[[PrimaryIdentifier]] is _recordTwo_.[[PrimaryIdentifier]], return *true*.</ins>
1. <ins>Else,</ins>
  1. <ins>If _offsetNanosecondsOne_ is not ~empty~ and _offsetNanosecondsTwo_ is not ~empty~ and _offsetNanosecondsOne_ = _offsetNanosecondsTwo_, return *true*.</ins>
1. Return *false*.

[justingrant]

I opened https://github.com/tc39/proposal-canonical-tz/pull/40 to make the change recommended above. Please review. Thanks!

[justingrant]

This proposal reached Stage 3 at the July 2023 TC39 meeting. As decided in the meeting, the next step is to merge this proposal into Temporal Stage 3 via https://github.com/tc39/proposal-temporal/pull/2633. Thanks for the reviews and for your support of this proposal!