Skip to content
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: Disallow one day long time zone offsets #2260

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

anba
Copy link
Contributor

@anba anba commented Jun 8, 2022

No description provided.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #2260 (f5b51cb) into main (a3a8237) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2260   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files          20       20           
  Lines       10803    10803           
  Branches     1925     1925           
=======================================
  Hits        10264    10264           
  Misses        503      503           
  Partials       36       36           
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.29% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb
Copy link
Member

ljharb commented Jun 8, 2022

Is this a timezone offset from UTC? If so, it can only range from -12 to +14 hours. If it's the difference between any two timezones, it could be as much as 26 hours.

@anba
Copy link
Contributor Author

anba commented Jun 8, 2022

It is the offset for a single time zone at a specific instant. The built-in time zone object supports up-to nsPerDay - 1 long offsets:

js> new Temporal.TimeZone("+23:59:59.999999999").getOffsetNanosecondsFor(new Temporal.Instant(0n))
86399999999999

@ljharb
Copy link
Member

ljharb commented Jun 8, 2022

Is the design intending to support possible future timezones with larger offsets than 14 hours? If so, why wouldn't "24 hours" be a valid value?

@anba
Copy link
Contributor Author

anba commented Jun 8, 2022

If so, it can only range from -12 to +14 hours.

The supported range is actually even longer because of LMT (local mean time):

const min = new Temporal.Instant(-86_40000_00000_00000_00000n);

let minOffset = +Infinity;
let maxOffset = -Infinity;
let minTimeZone;
let maxTimeZone;

for (let id of Intl.supportedValuesOf("timeZone")) {
  console.log(id);
  let tz = new Temporal.TimeZone(id);

  let instant = min;
  for (let i = 0; i < 1000; ++i) {
    let offset = tz.getOffsetNanosecondsFor(instant);

    if (offset < minOffset) {
      minOffset = offset;
      minTimeZone = id;
    }
    if (offset > maxOffset) {
      maxOffset = offset;
      maxTimeZone = id;
    }

    let next = tz.getNextTransition(instant);
    if (next === null) {
      break;
    }
    instant = next;
  }
}
console.log("minOffset:", minOffset, minTimeZone);
console.log("maxOffset:", maxOffset, maxTimeZone);

@ljharb
Copy link
Member

ljharb commented Jun 8, 2022

Is it up to 23 hours and 59 minutes, or a smaller value?

I'm trying to understand why 24 hours is a magic illegal number but "24 hours - 1 minute" is fine.

@anba
Copy link
Contributor Author

anba commented Jun 8, 2022

Is it up to 23 hours and 59 minutes, or a smaller value?

The maximum for built-in time zone objects is "+23:59:59.999999999" (and "-23:59:59.999999999" in the other direction). This can happen only when the time zone is created from an offset string.

@ljharb
Copy link
Member

ljharb commented Jun 8, 2022

Thanks, it all makes sense now :-)

@anba anba mentioned this pull request Jun 17, 2022
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch.

Would you mind splitting out the editorial change into a separate pull request so we can merge it ahead of presenting the normative change?

@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal labels Jul 4, 2022
@ptomato ptomato marked this pull request as draft July 4, 2022 21:25
Built-in time zones don't support an offset of `nsPerDay` nanoseconds and there
isn't any reason user-defined time zones should be able to support time zones
offsets of `nsPerDay` nanoseconds.
ptomato added a commit to ptomato/test262 that referenced this pull request Sep 16, 2022
…ndsFor

This tests the normative change from
tc39/proposal-temporal#2260
which achieved consensus in the July 2022 TC39 meeting.

The return value from a userland getOffsetNanosecondsFor method is no
longer allowed to be exactly one 24-hour day.
@ptomato
Copy link
Collaborator

ptomato commented Sep 16, 2022

Tests are in tc39/test262#3669.

Ms2ger pushed a commit to tc39/test262 that referenced this pull request Sep 16, 2022
…ndsFor

This tests the normative change from
tc39/proposal-temporal#2260
which achieved consensus in the July 2022 TC39 meeting.

The return value from a userland getOffsetNanosecondsFor method is no
longer allowed to be exactly one 24-hour day.
This is the corresponding change to the polyfill that was changed in the
spec text in f239577.
@ptomato ptomato force-pushed the GetOffsetNanosecondsFor-one-day branch from 658df46 to f5b51cb Compare September 16, 2022 17:44
@ptomato
Copy link
Collaborator

ptomato commented Sep 16, 2022

This achieved consensus in the July 2022 TC39 plenary meeting. I've added a commit that updates the reference code, and pulls in the new test262 tests.

@ptomato ptomato marked this pull request as ready for review September 16, 2022 18:15
@ptomato ptomato merged commit 0535c58 into tc39:main Sep 16, 2022
catamorphism pushed a commit to catamorphism/test262 that referenced this pull request Oct 4, 2022
…ndsFor

This tests the normative change from
tc39/proposal-temporal#2260
which achieved consensus in the July 2022 TC39 meeting.

The return value from a userland getOffsetNanosecondsFor method is no
longer allowed to be exactly one 24-hour day.
catamorphism pushed a commit to catamorphism/test262 that referenced this pull request Oct 5, 2022
…ndsFor

This tests the normative change from
tc39/proposal-temporal#2260
which achieved consensus in the July 2022 TC39 meeting.

The return value from a userland getOffsetNanosecondsFor method is no
longer allowed to be exactly one 24-hour day.
@anba anba deleted the GetOffsetNanosecondsFor-one-day branch August 4, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants