-
Notifications
You must be signed in to change notification settings - Fork 107
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
Normative: Allow UTC offset time zones #788
Conversation
spec/datetimeformat.html
Outdated
<h1>FormatTimeZoneOffsetString ( _offsetNanoseconds_ )</h1> | ||
<emu-alg> | ||
1. Assert: _offsetNanoseconds_ is an integer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend a structured header (and then there's no need for the assertion). We can also limit the type of offsetNanoseconds to "an integer in the range of -86,400,000,000,000 to 86,400,000,000,000". (I forget whether the range is inclusive or exclusive at both ends.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation is defined in Temporal and will be removed from ECMA-402 and/or this PR once added to ECMA-262. I will propagate any upstream changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
1. If the result of IsValidTimeZoneName(_timeZone_) is *false*, then | ||
1. Throw a *RangeError* exception. | ||
1. If IsTimeZoneOffsetString(_timeZone_) is *true*, then | ||
1. Let _offsetNanoseconds_ be ParseTimeZoneOffsetString(_timeZone_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a new CanonicalizeTimeZoneOffsetString AO in Temporal. Should this be brought into 402 to replace this line and the one below it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep changes minimal here, but intend to make use of CanonicalizeTimeZoneOffsetString once it lands in ECMA-262.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why not just change the definition of HourSubcomponents in ECMA262 instead
Currently, it is
HourSubcomponents[Extended] :::
TimeSeparator[?Extended] MinuteSecond
TimeSeparator[?Extended] MinuteSecond TimeSeparator[?Extended] MinuteSecond TemporalDecimalFractionopt
why not change it to just
HourSubcomponents[Extended] :::
TimeSeparator[?Extended] MinuteSecond
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place reference to it is UTCOffset and the only places refence to UTCOffset are
21.4.1.33.1 IsTimeZoneOffsetString ( offsetString )
and
21.4.1.33.2 ParseTimeZoneOffsetString ( offsetString )
which are exactly the two places we need to change and both places are only used by place dealing with system timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FrankYFTang An ECMA-262 grammar cannot be changed from an ECMA-402 pull request. But all of this will be simplified by/in response to Temporal, as demonstrated in #788 (comment) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I do not understand is why do we have this PR for ECMA402 instead of just let Temporal to change both ECMA262 and 402. What is the urgent need for that to happen before Temporal reach stage 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, the main reason is that offset time zones are currently supported in (pre-Temporal) ECMA-262. My assumption is that all ECMA-262 features should be supported when ECMA-402 is also implemented.
Also, do we know yet whether ICU and/or CLDR changes will be needed to enable formatting of offset time zones? If no, that's great news. If yes, then where do changes need to be made?
Richard point out https://www.ietf.org/archive/id/draft-ietf-sedate-datetime-extended-08.html require nanoseconds in the TimeZone but I cannot see from that spec of such requirement. what I see is
date-time and time-numoffset are imported from Section 5.6 of [RFC3339]
so.... where is the nanosecond requirement? |
It will be discussed upstream at tc39/ecma262#3087 and tc39/proposal-temporal#2593 . |
TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-06-01.md#normative-allow-utc-offset-time-zones-788 We need to resolve the minutes/seconds/centiseconds/nanoseconds first, but it seems okay to the group besides that point, which was somewhat controversial for a number of reasons. |
TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-06-29.md#normative-allow-utc-offset-timezones We'd like to wait until the Temporal changes land and then revisit this PR after it has been updated. |
@sffc Let's please also discuss it at the next TG2 meeting regardless. |
|
Do we support formatting these with timeZoneName "shortOffset" and "longOffset"? We should think about the formatting fallback given the six time zone format options: "short", "long", "shortOffset", "longOffset", "shortGeneric", "longGeneric". We discussed the fallback between these at length in the time zone format options proposal a couple of years ago. It shows up in BasicFormatMatcher: https://tc39.es/ecma402/#sec-basicformatmatcher |
Please write some example of the timeZone which previous throw but after this PR will be accepted . Thanks |
PR updated to remove support for UTC offsets with sub-minute precision, just like Temporal. |
@FrankYFTang added to the PR description. |
…ffset This aligns with ECMA-262, and therefore removes the DefaultTimeZone override.
673ca2e
to
26ba452
Compare
TG2 approval pending additional review of the spec text: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-09-07.md#normative-allow-utc-offset-time-zones-788 |
1. If IsTimeZoneOffsetString(_timeZone_) is *true*, then | ||
1. Let _parseResult_ be ParseText(StringToCodePoints(_timeZone_), |UTCOffset|). | ||
1. Assert: _parseResult_ is a Parse Node. | ||
1. If _parseResult_ contains more than one |MinuteSecond| Parse Node, throw a *RangeError* exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how does "contains more than one |MinuteSecond| Parse Node" work here.
How is the "contains" operation defined for Parse Node? I cannot find a concrete definition.
Look at the UTCOffset grammar in https://tc39.es/ecma262/#sec-time-zone-offset-string-format
UTCOffset :::
TemporalSign Hour
TemporalSign Hour HourSubcomponents[+Extended]
TemporalSign Hour HourSubcomponents[~Extended]
So if the concept of "contains" is shallow, then it would be impossible for UTCOffset to contain any MinuteSecond.
since 1 or 2 MinuteSecond could be contain inside the HourSubcomponents that contained by UTCOffset. However, the require a "deep containment" interpretation and I am not sure that is what ECMA262 specify. (since I cannot find a clear definition of what "contains" mean for Parse Node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FrankYFTang This use of "contains" is prose of the sort that is also currently in Temporal ParseTimeZoneIdentifier—as suggested by the lack of automatic hyperlinking, there is no concrete definition. However, as I thought would be clear from context in both documents, it is intended to be understood as entailing deep inspection of the tree of Parse Nodes. Can you suggest a change here that would make that more clear?
I could define a precise syntax-directed operation, but really don't want to do that because it would apply cross-document to the ECMA-262 |UTCOffset| nonterminal that will be replaced by Temporal |TimeZoneUTCOffsetName| (creating a situation where this part of ECMA-402 would be incoherent until fixed).
It's also worth noting that this explicit check for multiple |MinuteSecond| Parse Nodes will itself be removed when Temporal lands, along with many of the other steps here that are subsumed by ParseTimeZoneIdentifier:
+ 1. Let _timeZoneRecord_ be ? ParseTimeZoneIdentifier(_timeZone_).
- 1. If IsTimeZoneOffsetString(_timeZone_) is *true*, then
- 1. Let _parseResult_ be ParseText(StringToCodePoints(_timeZone_), |UTCOffset|).
- 1. Assert: _parseResult_ is a Parse Node.
- 1. If _parseResult_ contains more than one |MinuteSecond| Parse Node, throw a *RangeError* exception.
- 1. Let _offsetNanoseconds_ be ParseTimeZoneOffsetString(_timeZone_).
- 1. Let _offsetMinutes_ be _offsetNanoseconds_ / (6 × 10<sup>10</sup>).
- 1. Assert: _offsetMinutes_ is an integer.
+ 1. If _timeZoneRecord_.[[Name]] is ~empty~, then
+ 1. Let _offsetMinutes_ be _timeZoneRecord_.[[OffsetMinutes]].
1. Set _timeZone_ to FormatOffsetTimeZoneIdentifier(_offsetMinutes_).
- 1. Else if IsValidTimeZoneName(_timeZone_) is *true*, then
- 1. Set _timeZone_ to CanonicalizeTimeZoneName(_timeZone_).
+ 1. Else if IsValidTimeZoneName(_timeZoneRecord_.[[Name]]) is *true*, then
+ 1. Set _timeZone_ to CanonicalizeTimeZoneName(_timeZoneRecord_.[[Name]]).
1. Else,
1. Throw a *RangeError* exception.
This comment was marked as off-topic.
This comment was marked as off-topic.
sorry, I click the wrong bottom. |
Trying to implement in v8 https://chromium-review.googlesource.com/c/v8/v8/+/4854730 |
This comment was marked as off-topic.
This comment was marked as off-topic.
ICU SimpleTimeZone created by TimeZone::createInstance will handle the offset timezone but use id prefix with "GMT" |
One problem we have with the implementation is we can take the input timezone as but we it will be inefficient to distinguish it from "UTC" from the resolvedOptions. Could we treat them as simply as "UTC" in the spec text? (so resolvedOptions().timeZone will return "UTC" for these cases. |
please take a look at tc39/test262#3917 "+00" But after I dig into it more I think the current PR will make them all become timeZone "+00:00" and that should be fine. |
My understanding is for the following code
should get the following result: Please comment if my understanding is incorrect. thanks |
Nope. -00, -00:00, -0000, +00, and +0000 should all be normalized to +00:00. Here's why: String IDs of built-in time zones (either offset or IANA) that are returned by ECMAScript methods (including getters) should always be normalized. For offset zones, normalized means ±HH:MM, with -00:00 not allowed. For IANA time zones, normalized means matching the letter case used in the IANA time zone database. ECMAScript methods should accept non-normalized formats like +00 or -00:00, but should never return a time zone ID string except in a normalized format. This normalization behavior is specified in https://tc39.es/proposal-temporal/#sec-time-zone-identifiers, as well as in AO spec text in both Temporal and this PR. (If you find a case where an ECMAScript method is allowed to return a non-normalized ID for a built-in offset time zone or built-in named time zone, then it's a spec bug.) The reason why we require normalized outputs is so that implementations won't have to store the original ID strings that users provided. Instead, implementations are free to store timezone ID slots in a more optimized way, as long as the normalized ID string can be reconstituted later as needed. (e.g. in For example, an implementation could optimize storage of a timezone ID slot using a 16-bit union:
Hopefully this explains why "-00" => "+00:00" is the expected behavior. |
I assume that we want
So unless there's a very large performance advantage of making this change, then I'd like to leave the current spec as-is so that +00:00 and UTC are treated as distinct time zones, even though they always result in the same numeric results. |
I drop my early concern about "+00:00" vs "UTC". It is not a problem. |
I understand this PR is about letting an author specify a time zone with a simple string, as an offset to UTC, in the context of Temporal. I withdraw the concern I had during last meeting. Temporal.TimeZone has all functions for the use cases I had in mind. However we should then stick to the documentation for
The present polyfill gives also the seconds: |
The documentation is wrong. If a built-in IANA time zone (or a custom time zone that inherits from a built-in IANA zone) has sub-minute offsets, then I'm working on a PR (EDIT: tc39/proposal-temporal#2674) of those docs to fix the problem. |
This PR reach consensus on 2023-09-26 meeting |
Is this still blocked on something or should I hit merge? |
* Add test for ECMA402 PR 788 tc39/ecma402#788 * Fix misunderstanding about "+00:00" * Fix lint * Swap actual, expected position * Update test/intl402/DateTimeFormat/prototype/resolvedOptions/offset-timezone-change.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/resolvedOptions/offset-timezone-basic.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/formatToParts/offset-timezone-correct.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/constructor-invalid-offset-timezone.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/constructor-invalid-offset-timezone.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/resolvedOptions/offset-timezone-change.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/constructor-invalid-offset-timezone.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/format/offset-timezone-gmt-same.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/format/offset-timezone-gmt-same.js Co-authored-by: Richard Gibson <[email protected]> * Update test/intl402/DateTimeFormat/prototype/formatToParts/offset-timezone-correct.js Co-authored-by: Richard Gibson <[email protected]> --------- Co-authored-by: Richard Gibson <[email protected]>
please merge |
Congratulations everyone for getting this over the finish line! |
Fixes #683
This aligns with ECMA-262, and therefore removes the DefaultTimeZone override.
Example time zones that will now be accepted: Temporal |TimeZoneUTCOffsetName|