-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add lifetime-of-agent note in CanonicalizeTimeZoneName #2493
Conversation
The CanonicalizeTimeZoneName AO should be stable across the lifetime of the agent, like other TZ info operations.
Codecov Report
@@ Coverage Diff @@
## main #2493 +/- ##
==========================================
+ Coverage 94.94% 95.49% +0.55%
==========================================
Files 20 20
Lines 10821 10923 +102
Branches 1957 2031 +74
==========================================
+ Hits 10274 10431 +157
+ Misses 487 430 -57
- Partials 60 62 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 might be normative in the most strict sense of that term (consider e.g. Temporal.TimeZone.from(str).id
), but it is in line with the content of Note 2 at ToLocalTime (which should itself move out of the note so it can be normative).
Also, if we're making this fix, can we also tighten the related constraints of GetNamedTimeZone{Previous,Next}Transition?
-Given the same values of _timeZoneIdentifier_ and _epochNanoseconds_, the result must be the same for the lifetime of the surrounding agent.
+Given the same value of _timeZoneIdentifier_, results must remain consistent for the lifetime of the surrounding agent.
+Specifically, the result must be the same for _epochNanoseconds_ and all {lesser,greater} values of _epochNanoseconds_ except (for a non-*null* result) those that are {less,greater} than or equal to that result.
+In other words, implementations are prohibited from inserting a transition where ECMAScript code has already observed its absence.
Should we tighten all other constraints too? For example, the constraint of ToLocalTime says:
|
What would a tighter form of that look like? |
The particular failure case I had in mind was that the language above only guarantees consistency if |
I think our best bet would be to just rewrite ToLocalTime altogether to use the GetNamedTimeZone... stuff and the builtin Calendar methods. However, that seems to me better to do in consultation with TG2, maybe after Temporal lands. |
Stepping back, it seems like the current spec text takes a one-AO-at-a-time approach that seems both needlessly complex and also allows for interesting failure cases, like having different TZDATA versions used for Maybe we could have a simpler and more global requirement? I think the result we want is that, for the lifetime of the agent:
Is this indeed what we want? If yes, what's a good way to put this in the spec? |
I think CreateTemporalTimeZone would be a nice place to move the text currently in Note 2 of ToLocalTime for covering most cases, and then ToLocalTime itself can reference the text (but probably should be refactored a bit, because right now it isn't even clear what belongs in the fields for non-Gregorian calendars). |
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.
How should we move this forward? I think the PR is an improvement as is, I could go either way on including the other improvements discussed in the comments now or later. (I think ToLocalTime probably should be reworded in ECMA-402 even before Temporal lands though.)
@ptomato @gibson042 what do you guys think about the commit that I just pushed which adds a new section to explain the requirements about consistency and updates? Then we could link to this section from various places where time zone info is accessed. |
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 makes sense to me. I'd like to get Richard's opinion on whether this is sufficiently "spec-ese" language. For example, we might want to go into less detail about why implementations might update dynamically and more detail about exactly which operations must return consistent values. ("equivalent time zone data" might not be precise enough.)
e4f973b
to
c02b446
Compare
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 like the new section! Some suggested tweaks for clarity and precision and then LGTM.
<p> | ||
The IANA Time Zone Database is frequently updated, averaging 10 annual releases in recent years. | ||
An ECMAScript implementation is strongly encouraged to use the most recent version of the IANA Time Zone Database that was available when the implementation was released. | ||
</p> | ||
<p> | ||
It is also acceptable for implementations to update time zone data dynamically, either directly by loading a new version of the IANA Time Zone Database or indirectly by calling an external, dynamically-updated source like an operating system. | ||
It is allowed for these updates to happen during the lifetime of the surrounding agent. | ||
However, to ensure that ECMAScript programs can depend on a consistent view of time zone data, this specification places the following requirements on ECMAScript implementations that dynamically update time zone data: | ||
</p> |
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 don't think we should recommend baking in tzdata over dynamic loading, especially since that's really not how most implementations actually behave.
<p> | |
The IANA Time Zone Database is frequently updated, averaging 10 annual releases in recent years. | |
An ECMAScript implementation is strongly encouraged to use the most recent version of the IANA Time Zone Database that was available when the implementation was released. | |
</p> | |
<p> | |
It is also acceptable for implementations to update time zone data dynamically, either directly by loading a new version of the IANA Time Zone Database or indirectly by calling an external, dynamically-updated source like an operating system. | |
It is allowed for these updates to happen during the lifetime of the surrounding agent. | |
However, to ensure that ECMAScript programs can depend on a consistent view of time zone data, this specification places the following requirements on ECMAScript implementations that dynamically update time zone data: | |
</p> | |
<p> | |
The IANA Time Zone Database is frequently updated, recently averaging 10 releases per year. | |
An ECMAScript implementation is strongly encouraged to always use the most recent version of time zone that is available to it, either built in to its released artefacts or loaded dynamically in whole or in part (such as through system calls provided by the host environment's operating system). | |
It is allowed for updates to occur during the lifetime of the surrounding agent. | |
However, to ensure that ECMAScript code can rely upon a consistent view of time zone data, this specification places the following requirements on the effects of dynamic updates: | |
</p> |
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.
Yep, I like those changes. My intent wasn't to stan baked-in data but instead to discourage shipping stale TZDB data that had already been superseded. Your text more clearly says what I was trying to say. Thanks!
dynamic loading, especially since that's really not how most implementations actually behave.
I naively assumed that browsers shipped with TZDB to avoid relying on out-of-date TZDB in the operating system. So does this mean that a brand-new release of Chrome that's running on, say, a pirated Windows version that's many years old, will have severely out-of-date TZ data? Seems kinda bad!
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 naively assumed that browsers shipped with TZDB to avoid relying on out-of-date TZDB in the operating system. So does this mean that a brand-new release of Chrome that's running on, say, a pirated Windows version that's many years old, will have severely out-of-date TZ data? Seems kinda bad!
As it turns out, my mental model was stale. It looks like at least current Firefox at does ship with tzdb, and likely also Chrome and other V8 browsers. Comments suggest that both use ICU rather than OS syscalls, but it wouldn't surprise me to find out that precise behavior actually varies across e.g. platform or possibly even the age of data.
Regardless, the text here is intended to be general enough for that to be an implementation choice while still ensuring sufficient consistency guarantees for ECMAScript code.
After time zone data for a particular canonical named time zone is used during the lifetime of the surrounding agent, equivalent time zone data must be used for the lifetime of the surrounding agent for all subsequent use of the same canonical time zone name. | ||
However, implementations are allowed to use different versions of time zone data for different canonical time zone names. |
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.
After time zone data for a particular canonical named time zone is used during the lifetime of the surrounding agent, equivalent time zone data must be used for the lifetime of the surrounding agent for all subsequent use of the same canonical time zone name. | |
However, implementations are allowed to use different versions of time zone data for different canonical time zone names. | |
After time zone data is used for a particular canonical time zone, equivalent data must be used in all subsequent interactions involving the same canonical time zone name for the lifetime of the surrounding agent. | |
However, implementations are allowed to use different versions of time zone data for different canonical time zone names. |
However, implementations are allowed to use different versions of time zone data for different canonical time zone names. | ||
</li> | ||
<li> | ||
Any time zone name string, when evaluated ASCII-case-insensitively, must be canonicalized to the same canonical time zone name for the lifetime of the surrounding agent. |
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.
Any time zone name string, when evaluated ASCII-case-insensitively, must be canonicalized to the same canonical time zone name for the lifetime of the surrounding agent. | |
Once IsAvailableTimeZoneName has returned *true* for a String, that String and any String that is an ASCII-case-insensitive match for it must be <emu-xref href="#sec-canonicalizetimezonename">canonicalized</emu-xref> to the same canonical time zone name for the lifetime of the surrounding agent. |
Co-authored-by: Richard Gibson <[email protected]>
Closing this PR in favor of #2573. |
The CanonicalizeTimeZoneName AO should be stable across the lifetime of the agent, like other TZ info operations such as 15.4.22 ToLocalTime.
My understanding was that TZ data that's specific to a single time zone is guaranteed to remain invariant for the lifetime of the surrounding agent. I believe that canonicalization should have been included in this guarantee, but was missed in the spec text.
Is this a normative change?