FrankYFTang's feedback about the current spec text

[FrankYFTang]

This is to reply Justin's email to me and shane after the Jun 29 2023 TG2 meeting:
Justin wrote:

Hi Frank - The broken links you saw yesterday are fixed. Let me know if you have any concerns with the proposal spec text, especially anything that would block Stage 3 advancement. Thanks!

Dear Justin
I have a lot of problem with your current spec text in this proposal

In all other proposal, we use <ins> and <del> in the spec text to illustrate the modification from the pre-existing ECMA402 or ECMA262However, when I look at your spec text, I do not see the <ins> and <del> is properly inserted to show the true CHANGES
In particular the changes to InitializeDateTimeFormat
In your spec text, the only <del> and <ins> for InitializeDateTimeFormat is step 29.e

e. Set timeZone to timeZoneIdentifierRecord.[[<del>PrimaryIdentifier</del><ins>Identifier</ins>]].

However, if we exam the current shape of InitializeDateTimeFormat in ECMA402https://tc39.es/ecma402/#sec-initializedatetimeformat

It is very different than what you proposed
In the current ECMA402, the process to resolve the TimeZone is
(see https://tc39.es/ecma402/#sec-initializedatetimeformat )

29. Let timeZone be ? [Get](https://tc39.es/ecma262/#sec-get-o-p)(options, "timeZone").
30. If timeZone is undefined, then
a. Set timeZone to [DefaultTimeZone](https://tc39.es/ecma402/#sup-defaulttimezone)().
31. Else,
a. Set timeZone to ? [ToString](https://tc39.es/ecma262/#sec-tostring)(timeZone).
b. If the result of [IsValidTimeZoneName](https://tc39.es/ecma402/#sec-isvalidtimezonename)(timeZone) is false, then
i. Throw a RangeError exception.
c. Set timeZone to [CanonicalizeTimeZoneName](https://tc39.es/ecma402/#sec-canonicalizetimezonename)(timeZone).
32. Set dateTimeFormat.[[TimeZone]] to timeZone.

but in your spec text, it is (see https://tc39.es/proposal-canonical-tz/#sec-temporal-initializedatetimeformat )

27. Let timeZone be ? [Get](https://tc39.es/ecma262/#sec-get-o-p)(options, "timeZone").
28. If timeZone is undefined, then
a. If toLocaleStringTimeZone is present, then
i. Set timeZone to toLocaleStringTimeZone.
b. Else,
i. Set timeZone to [SystemTimeZoneIdentifier](https://tc39.es/proposal-canonical-tz/#sec-systemtimezoneidentifier)().
29. Else,
a. If toLocaleStringTimeZone is present, throw a TypeError exception.
b. Set timeZone to ? [ToString](https://tc39.es/ecma262/#sec-tostring)(timeZone).
c. Let timeZoneIdentifierRecord be [GetAvailableNamedTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-getavailablenamedtimezoneidentifier)(timeZone).
d. If timeZoneIdentifierRecord is empty, then
i. Throw a RangeError exception.
e. Set timeZone to timeZoneIdentifierRecord.[[PrimaryIdentifierIdentifier]].
30. Set dateTimeFormat.[[TimeZone]] to timeZone.

Notice it is far more different than what you marked by <del> and <ins>.

I assume you are marking the <del> and <ins> from the Temporal proposal instead of the ECMA262 / ECMA402since the  Temporal spec text is (see https://tc39.es/proposal-temporal/#sec-temporal-initializedatetimeformat)

29. Let timeZone be ? [Get](https://tc39.es/ecma262/#sec-get-o-p)(options, "timeZone").
30. If timeZone is undefined, then
a. If toLocaleStringTimeZone is present, then
i. Set timeZone to toLocaleStringTimeZone.
b. Else,
i. Set timeZone to [SystemTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-systemtimezoneidentifier)().
31. Else,
a. If toLocaleStringTimeZone is present, throw a TypeError exception.
b. Set timeZone to ? [ToString](https://tc39.es/ecma262/#sec-tostring)(timeZone).
c. Let timeZoneIdentifierRecord be [GetAvailableNamedTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-getavailablenamedtimezoneidentifier)(timeZone).
d. If the result of [IsValidTimeZoneName](https://tc39.es/proposal-temporal/#sec-isvalidtimezonename)(timeZone) is falsetimeZoneIdentifierRecord is empty, then
i. Throw a RangeError exception.
e. Set timeZone to [CanonicalizeTimeZoneName](https://tc39.es/proposal-temporal/#sec-canonicalizetimezonename)(timeZone)timeZoneIdentifierRecord.[[PrimaryIdentifier]].
32. Set dateTimeFormat.[[TimeZone]] to timeZone.

I feel it is troublesome to advance a proposal to Stage 3 based on another unstable Stage 3 proposal (sorry to say this, but I still do not believe the Temporal proposal is stable yet by just looking at the number of lines changed in https://github.com/tc39/proposal-temporal/commits/main the last 2 months)

But let's first ignore that for a while but just look what you changed to the impact to the InitializeDateTimeFormat w/o supporting Temporal
In the current shape of ECMA402, if the timezone is undefined, the timezone is set toDefaultTimeZone().
which according to  https://tc39.es/ecma402/#sup-defaulttimezone:

 "6.5.3 DefaultTimeZone ( )The implementation-defined abstract operation DefaultTimeZone takes no arguments and returns a String. It returns a String value representing the host environment's current time zone, which is a valid (6.5.1) and canonicalized (6.5.2) time zone name.This definition supersedes the definition provided in es2024, 21.4.1.10."

Notice there are NO mandate for the engine to support Timezone such as "Etc/GMT+1:12:33"

Now, what Temporal proposal said about this?

The Temporal propose currently change the spec text to set to SystemTimeZoneIdentifier() and the SystemTimeZoneIdentifier is defined in https://tc39.es/proposal-temporal/#sec-systemtimezoneidentifier as

"14.9.3 SystemTimeZoneIdentifier ( )
The implementation-defined abstract operation SystemTimeZoneIdentifier takes no arguments and returns a String. It returns a String representing the host environment's current time zone, which is either a String representing a UTC offset for which IsTimeZoneOffsetString returns true, or a primary time zone identifiera primary time zone identifier or an offset time zone identifier. It performs the following steps when called:

1. If the implementation only supports the UTC time zone, return "UTC".
2. Let systemTimeZoneString be the String representing the host environment's current time zone, either: a primary time zone identifier; or an offset time zone identifier.
3. Return systemTimeZoneString.

"Notice with that definition, there are still NO mandate for the engine to support A TimeZone such as "Etc/GMT+1:12:33".

However, with the spec text in your proposal, you change the definition of SystemTimeZoneIdentifier to the following (see https://tc39.es/proposal-canonical-tz/#sec-systemtimezoneidentifier)
"
1.1 SystemTimeZoneIdentifier ( )
The implementation-defined abstract operation SystemTimeZoneIdentifier takes no arguments and returns a String. It returns a String representing the host environment's current time zone, which is either a primary time zone identifier or an offset time zone identifier.

EDITOR'S NOTE
Updates to this abstract operation overlap with a Temporal PR (#2607 - Normative: Limit offset time zones to minutes precision) being presented in the July 2023 TC39 meeting. If that PR is approved, SystemTimeZoneIdentifier changes will be removed from this proposal.

It performs the following steps when called:

1. If the implementation only supports the UTC time zone, return "UTC".
2. Let systemTimeZoneString be the String representing the [host environment](https://tc39.es/ecma262/#host-environment)'s current time zone, either a [primary time zone identifier](https://tc39.es/ecma262/#sec-time-zone-identifiers) or an [offset time zone](https://tc39.es/ecma262/#sec-time-zone-identifiers) identifier.
3. Let offsetNanoseconds be ! [ParseTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-parsetimezoneidentifier)(systemTimeZoneString).
4. If offsetNanoseconds is not empty, return [FormatOffsetTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-temporal-formatoffsettimezoneidentifier)(offsetNanoseconds, separated).
5. Return systemTimeZoneString.

"

This change the SystemTimeZoneIdentifier to support a timezone such as "Etc/GMT+1:12:33" or even
"Etc/GMT+1:12:33.123456789"

and I am strongly oppose this. I understand there is another issue to change Temporal to only accept to minutes offset, however, that part is not yet in ECMA262, ECMA402 nor Temporal proposal and it is very troublesome to advance a spec text not only depend on an unstable Stage 3 proposal but also some possible changes toward that unstable proposal.

As I mentioned, If you remove the changes to SystemTimeZoneIdentifier I am ok for that to advance to Stage 3 at this point
But based on the fact that w/ what already in ECMA262, ECMA402 and already in Temporal ONLY, your current spec text change the requirement for Intl.DateTimeFormat drmatically that we simpley cannot implement

(try to run TZ="Etc/GMT+1:12:33.123456789" chrome on Linux, it currently will have "Etc/Unknown" and I do not think it is needed to support that nor know a way to implement the support for that)

I will block this proposal to Stage 3 because of this.

[FrankYFTang]

Another non blocking review issue

In
https://tc39.es/proposal-canonical-tz/#sec-temporal-timezoneequals

10. If recordOne is not empty and recordTwo is not empty and recordOne.[[PrimaryIdentifier]] is recordTwo.[[PrimaryIdentifier]], return true.

Notice recordOne and recordTwo are both result of ToTemporalTimeZoneIdentifier(one).

and the return value of ToTemporalTimeZoneIdentifier based on the assertion of

a. [Assert](https://tc39.es/ecma262/#assert): Either [IsOffsetTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-isoffsettimezoneidentifier)(timeZoneSlotValue) is true, or [GetAvailableNamedTimeZoneIdentifier](https://tc39.es/proposal-temporal/#sec-getavailablenamedtimezoneidentifier)(timeZoneSlotValue) is not empty.

has only two possibility if it is string
and if it is not a string, but return from the get "id" it is also one of the possible two

and in your step

7. If offsetNanosecondsOne is not empty or offsetNanosecondsTwo is not empty, then

then we know before step 8 we must have

1. [Assert](https://tc39.es/ecma262/#assert): offsetNanosecondsOne is empty.
1. [Assert](https://tc39.es/ecma262/#assert): offsetNanosecondsTwo is empty.

and therefore before step 10 it must also be

Assert(recordOne is not empty)
Assert(recordTwo is not empty)

So I suggest you make the following modification
A. Add

[Assert](https://tc39.es/ecma262/#assert): offsetNanosecondsOne is empty.
[Assert](https://tc39.es/ecma262/#assert): offsetNanosecondsTwo is empty.

between Step 7 and 8

B. Add

[Assert](https://tc39.es/ecma262/#assert): recordOne is not empty.

after step 8
and add

[Assert](https://tc39.es/ecma262/#assert): recordTwo is not empty.

after step 9

C. Simplified step 10 to

1. If recordOne.[[PrimaryIdentifier]] is recordTwo.[[PrimaryIdentifier]], return true.

[FrankYFTang]

To illustrate the blocking issue. Currently, on linux to run chrome for the following

(new Intl.DateTimeFormat()).resolvedOptions().timeZone

If I start the chrome with "TZ="Etc/GMT+3" /opt/google/chrome/chrome"
I will get
"Etc/GMT+3"

Your proposal mandate me to support if I start my chrome with
$ TZ="Etc/GMT+3:29:12.987654321" /opt/google/chrome/chrome
to return
"Etc/GMT+3:29:12.987654321" for "(new Intl.DateTimeFormat()).resolvedOptions().timeZone"

which I believe that is unneeded by any real users in this world and unimplementable by what currently v8 depends on.

Therefore I will block it for Stage 3.

[justingrant]

I assume you are marking the <del> and <ins> from the Temporal proposal

Yep, that's correct, and is noted in the spec:

Our intent was to make it easier to understand what this proposal is changing, because most of the AOs changed by this proposal only have a one-line change.

It made more sense to use Temporal's spec text as this proposal's base text, because we assumed that this proposal will not reach Stage 4 before Temporal. Making this assumption official is one of the discussion topics in our Stage 3 advancement presentation.

Your proposal mandate me to support if I start my chrome with
$ TZ="Etc/GMT+3:29:12.987654321" /opt/google/chrome/chrome

This was not our intent. The text in this proposal's SystemTimeZoneIdentifier is only intended to ensure that strings like "+0300" and "+03" are never returned from SystemTimeZoneIdentifier, but are instead transformed to an offset string's normalized form which is "+03:00".

The goal is to ensure that implementations are free to store offset time zones as signed integers, instead of having to store the actual string that the OS gave you. This is analogous to the way this proposal requires new Temporal.TimeZone("asia/kolkata").id to return the case-normalized "Asia/Kolkata", so that available named time zone IDs can be stored as a 10-bit index or enumeration instead of the user's original string.

Note that tc39/proposal-temporal@a42c2fe, which is part of tc39/proposal-temporal#2607 (the PR you requested to limit offset zone precision to minutes only) also removes the need for this proposal to change SystemTimeZoneIdentifier.

The plan is for Temporal to present before this proposal at the July TC39 meeting, and if that PR is accepted then I'll remove SystemTimeZoneIdentifier changes from this proposal before presenting it to the committee.

But in the meantime, I'm happy to change the SystemTimeZoneIdentifier text in this proposal to any other text that you think would best convey the normalization intent described above. Would a note suffice?

If not, then I can simply remove the SystemTimeZoneIdentifier changes and assume that tc39/proposal-temporal#2607 will be accepted. Given that PR was requested from you and other implementers and it's clear that Temporal won't get to Stage 4 without it, it seems safe to assume that this PR will be accepted.

Let me know which option (note or removal) you'd prefer.

Note that offset time zones don't use syntax like "Etc/GMT+3:29". A valid syntax according to ECMA-262's |UTCOffset| production would be any of the following:

+03:00
+0300
+03
+03:00:12
+03:00:12.987654321

Assuming that tc39/proposal-temporal#2607 removes support for sub-minute units, the last two examples above will become invalid to Temporal (and to ECMA-262 after Temporal reaches Stage 4).

It may make sense also file a "web reality" PR to ECMA-262 that changes ECMA-262's |UTCOffset| too, even before Temporal reaches Stage 4. What do you think?

@gibson042's PR to add offset time zone support to ECMA-402 (tc39/ecma402#788) will also be modified to use the same limit to minutes only.

I'll respond to your other feedback in a subsequent comment, but first I wanted to ensure we're working with a common understanding about the SystemTimeZoneIdentifier changes. Thanks!

[FrankYFTang]

Note that offset time zones don't use syntax like "Etc/GMT+3:29". A valid syntax according to ECMA-262's |UTCOffset| production would be any of the following:

+03:00
+0300
+03
+03:00:12
+03:00:12.987654321

ok, you are right, I should read the grammar more carefully, it does not have the "Etc/GMT" prefix for those cases

Could you explain to me why the system timezone could have any of the following?

+03:00
+0300
+03
+03:00:12
+03:00:12.987654321

Which OS support that? and which other program also support these TimeZone as system timezone?
I could understand such value came from the network from a value defined by RFC or ISO8601 but we are talking about the system timezone here right? In what case we will see such value as in 2023? Do we really need this change in SystemTimeZoneIdentifier ? why do we need to support "an offset time zone identifier" as possible value in SystemTimeZoneIdentifier but not just "a valid (6.5.1) and canonicalized (6.5.2) time zone name." as described in https://tc39.es/ecma402/#sup-defaulttimezone ?

[justingrant]

and if it is not a string, but return from the get "id" it is also one of the possible two

If I'm understanding your feedback correctly, then I think the current text is OK.

For built-in time zones, you are correct: the result of the id getter is always a valid offset time zone ID or an available time zone identifier.

But for custom time zone objects, it's possible for id to be a string like "Fake/Time_Zone", which would cause GetAvailableNamedTimeZoneIdentifier to return ~empty~. That's why we need is not ~empty~ checks and can't rely on assertions.

Or were you suggesting something else that I'm not understanding?

[FrankYFTang]

ok I did some more testing. It seems in MacOS the date command will respect TZ="+05:15"

in that case I am convinced that we should include the support for SystemTimeZoneIdentifer for the following cases

+03:00
+0300
+03

but NOT

+03:00:12
+03:00:12.987654321

[justingrant]

Cool, thanks for testing that. I think we're agreeing now? Assuming tc39/proposal-temporal#2607 is approved, then both Temporal and this proposal will allow +03:00, +0300, and +03 but disallow +03:00:12 and +03:00:12.987654321 from all places where time zone identifiers are accepted as input, including when that input comes from the OS in SystemTimeZoneIdentifier.

Would you still prefer that I remove the changes to SystemTimeZoneIdentifier in this proposal's text?

[FrankYFTang]

Would you still prefer that I remove the changes to SystemTimeZoneIdentifier in this proposal's text?

Assuming that is agreed, I will not block this proposal even if you keep SystemTimeZoneIdentifier as is. I can always give an implementation feedback to ask for removal if that is not pass and I will express implementation difficulty to implement that. (but I assume that will not happen)

[FrankYFTang]

But for custom time zone objects

oh... I see. For all other Temporal.*.prototype.equals you always perform a ToTemporalXX to the other before compare. Somehow in this case we compare directly

This mean if have the following code

let tzid = "Asia/Taipei";
let obj = {id: tzid};
let tz1 = Temporal.TimeZone.from(tzid);
tz1.equals(obj); //  => true

right?
but is that what you like to see?

[justingrant]

Yep, that's how TimeZoneEquals works in current Temporal, and this proposal doesn't change it.

The only difference in this proposal is that a canonicalization step is added before the final ID comparison happens.

In the current Temporal spec, canonicalization happens when the TimeZone instance is constructed, but does not happen in TimeZoneEquals, so if a custom calendar's id returns a non-canonical ID, then it will never match any built-in calendar in TimeZoneEquals.

In this proposal, if a custom calendar's id returns a non-canonical ID, then it will be canonicalized and then compared.

The code below illustrates TimeZoneEquals behavior in current Temporal vs. this proposal.

const offset = '+07:00';
const epoch = new Temporal.Instant(0n);
const offsetNanoseconds = Temporal.TimeZone.from(offset).getOffsetNanosecondsFor(epoch);
const getPossibleInstantsFor = dt => dt.toZonedDateTime(offset);
const getOffsetNanosecondsFor = () => offsetNanoseconds;

timeZone1 = { id: 'Asia/Ulaanbaatar', getOffsetNanosecondsFor, getPossibleInstantsFor };
zdt1 = new Temporal.ZonedDateTime(0n, timeZone1)
// => 1970-01-01T07:00:00+07:00[Asia/Ulaanbaatar]

timeZone2 = { id: 'Asia/Ulan_Bator', getOffsetNanosecondsFor,  getPossibleInstantsFor };
zdt2 = new Temporal.ZonedDateTime(0n, timeZone2);
// => 1970-01-01T07:00:00+07:00[Asia/Ulan_Bator]

timeZone3 = 'Asia/Ulan_Bator';
zdt3 = new Temporal.ZonedDateTime(0n, timeZone3);
// => 1970-01-01T07:00:00+07:00[Asia/Ulaanbaatar] (current Temporal)
// => 1970-01-01T07:00:00+07:00[Asia/Ulan_Bator] (proposal-canonical-tz)

zdt1.equals(zdt3)
// => true

zdt1.equals(zdt2)
// false (current Temporal)
// true (proposal-canonical-tz)

In current Temporal, zdt1.equals(zdt2) is false because the id getters don't match. In proposal-canonical-tz, TimeZoneEquals includes a canonicalization step, so zdt1.equals(zdt2) is true.

A reasonable question is whether ID comparison is the right way to determine if two time zones are equal. The short answer is that it's something we discussed a lot, and there wasn't any better way available to determine equality. So we're sticking with ID string equality as the least-bad way to determine time zone equality.

[justingrant]

Assuming that is agreed, I will not block this proposal even if you keep SystemTimeZoneIdentifier as is. I can always give an implementation feedback to ask for removal if that is not pass and I will express implementation difficulty to implement that. (but I assume that will not happen)

OK great! Thanks for your support. Feel free to let me know if you see any other problems in the proposal.

[anba]

ok I did some more testing. It seems in MacOS the date command will respect TZ="+05:15"

On Linux (tested on Ubuntu) it's possible to specify seconds precision:

~$ TZ="MyTimeZone+03:12:34" date "+%::z (%Z)"
-03:12:34 (MyTimeZone)

But that doesn't mean I feel like we need/should support seconds precision.

[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 tc39/proposal-temporal#2633. Thanks for the feedback, and I look forward to more feedback as this proposal is implemented. Closing this issue now. Thanks again!