Draft of custom time zone API #498

ptomato · 2020-04-06T18:52:28Z

Codecov Report

Merging #498 into main will increase coverage by 0.72%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #498      +/-   ##
==========================================
+ Coverage   96.83%   97.56%   +0.72%     
==========================================
  Files          17       17              
  Lines        3955     3986      +31     
  Branches      582      583       +1     
==========================================
+ Hits         3830     3889      +59     
+ Misses        123       95      -28     
  Partials        2        2

Flag	Coverage Δ
#test262	`51.99% <60.19%> (-0.42%)`	⬇️
#tests	`93.21% <97.35%> (+0.98%)`	⬆️

Impacted Files	Coverage Δ
polyfill/lib/absolute.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/date.mjs	`93.03% <100.00%> (+0.52%)`	⬆️
polyfill/lib/datetime.mjs	`95.10% <100.00%> (+3.41%)`	⬆️
polyfill/lib/duration.mjs	`98.18% <100.00%> (+4.49%)`	⬆️
polyfill/lib/ecmascript.mjs	`97.91% <100.00%> (-0.16%)`	⬇️
polyfill/lib/monthday.mjs	`98.83% <100.00%> (+0.20%)`	⬆️
polyfill/lib/now.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/time.mjs	`99.28% <100.00%> (+2.81%)`	⬆️
polyfill/lib/timezone.mjs	`87.91% <100.00%> (+1.20%)`	⬆️
polyfill/lib/yearmonth.mjs	`96.89% <100.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b251071...c011110. Read the comment docs.

sffc

In general, I think it is nicer to pass real Temporal objects where possible, in line with what we decided last week for #354. So instead of taking or returning BigInt epoch nanos, just use an Absolute instead.

The idToTimeZone thing should follow what we come up with for calendars in #294.

docs/timezone-draft.md

sffc · 2020-04-06T19:07:41Z

docs/timezone-draft.md

+
+> **FIXME:** This means we have to remove any checks for the _[[InitializedTemporalTimeZone]]_ slot in any APIs outside of `Temporal.TimeZone`.
+
+> **FIXME:** `Temporal.TimeZone` is supposed to be an iterable through all time zones known to the implementation, but what do we do about custom time zones there?


We should decide how to do this in #294 for calendars and apply the same solution here.

sffc · 2020-04-06T21:29:54Z

docs/timezone-draft.md

+
+  /** Given an absolute instant returns this time zone's corresponding
+   * UTC offset, in nanoseconds (signed). */
+  getOffsetAtInstant(absolute : Temporal.Absolute) : bigint;


It should return a Temporal.Absolute, right?

I suppose it could, but this is conceptually more of a duration than an absolute point on the timeline. I only didn't use Temporal.Duration because it can't represent a sign, and then I'd have to return a pair of { sign, duration }.

Oh, I see, it's an offset from UTC, not nanos since epoch. Never mind then; we should use a numeric type. It's an interesting question about whether to use bigint or number. Maybe open a thread to answer that outside of this PR.

It's fairly minor and easy to change if we change our minds — I'll just change it to a number for now. The allowed offsets range from -24:00 to +24:00, not including endpoints, and 86400e9 is less than Number.MAX_SAFE_INTEGER, so a bigint doesn't seem necessary.

docs/timezone-draft.md

Ms2ger

I feel like it would be valuable to list some of the more compelling use cases here. I'm not entirely sure that "maybe the time zone database is out of date" is sufficient reason for the complexity.

docs/timezone-draft.md

Ms2ger · 2020-04-09T07:53:52Z

docs/timezone-draft.md

+  /** Given the calendar/wall-clock time, returns an array of 0, 1, or
+   * 2 absolute instants that are possible points on the timeline


Do we want to enforce the maximum of two elements?

Do we want to enforce this returns an array, rather than an iterable?

Should this comment say something about how we'd pick one of them?

Do we want to enforce the maximum of two elements?

I don't know, I was wondering if it would theoretically be possible for a time zone to decide to have two DST transitions back-to-back so that a DateTime was repeated three times. That wouldn't be handled by getAbsoluteFor, since the disambiguation parameter only takes "earlier" or "later" into account, but possibleInstants could be used by user code to deal with that case.

If we did want to enforce the maximum then it might make more sense to return an object { earlier: time1, later: time2 } and just have getAbsoluteFor take the key that corresponds to the disambiguation parameter, or throw if the two are different in "reject" mode. That would move the responsibility for deciding what to do in the case of 0 absolutes found, from getAbsoluteFor into possibleInstants, and therefore into user code in the case of custom time zones.

Do we want to enforce this returns an array, rather than an iterable?

In the current form I'm not sure it makes sense to return an iterable, since the first thing we do is check its length.

Should this comment say something about how we'd pick one of them?

I'll add this.

Ms2ger · 2020-04-09T07:55:51Z

docs/timezone-draft.md

+Alternatively, a custom time zone doesn't have to be a subclass of `Temporal.TimeZone`.
+In this case, it can be a plain object, which should (must?) implement all of the non-static methods in the interface.


Haven't been following this particular discussion, so it might have been covered already – why is it desirable to allow this?

I based that on the discussion in #300.

Ms2ger · 2020-04-09T07:58:39Z

docs/timezone-draft.md

+Alternatively, a custom time zone doesn't have to be a subclass of `Temporal.TimeZone`.
+In this case, it can be a plain object, which should (must?) implement all of the non-static methods in the interface.
+
+> **FIXME:** This means we have to remove any checks for the _[[InitializedTemporalTimeZone]]_ slot in any APIs outside of `Temporal.TimeZone`.


Even the this checks in the methods on the prototype? That is, should we support Temporal.TimeZone.prototype.getOffsetFor.call({ getOffsetAtInstant() { ... } }, ...)? Why or why not?

When I wrote this I didn't intend to remove checks in the methods on Temporal.TimeZone.prototype, but I see in your example how that could be useful. If we did Temporal.TimeZone.prototype.getOffsetFor.call(possiblyPlainObject, absolute) internally then plain objects wouldn't have to implement all the methods. They'd only have to implement the methods that subclasses would have to implement. So I think we should support that.

docs/timezone-draft.md

ryzokuken · 2020-04-10T11:49:50Z

@ptomato could you please rebase this on top of #506 or master if it gets merged by the time you read this? Thanks.

Ms2ger · 2020-04-14T15:52:10Z

docs/timezone-draft.md

+
+These functions gain an `options` parameter with an `idToTimeZone` option, whose value is a function returning the appropriate time zone object for the `id` passed in, or `null` to indicate that the time zone doesn't exist.
+
+If the option is not given, `Temporal.TimeZone.idToTimeZone()` is called.


If it wasn't for the fact that it makes sense to match the idToTimeZone option, I'd say Temporal.TimeZone.fromId() would make more sense. Now I don't know. On the other hand, do we need this given that Temporal.TimeZone.from() exists?

I think we can hash this out in #294.

Ms2ger · 2020-04-14T15:58:42Z

docs/timezone-draft.md

+> **FIXME:** These names are not very good.
+> Help is welcome in determining the color of this bike shed.
+
+Alternatively, a custom time zone doesn't have to be a subclass of `Temporal.TimeZone`.


Another question: what happens if you call Temporal.TimeZone.from() with such a plain object?

Since the plain object must implement the name property, it would be the same result as calling Temporal.TimeZone.from(obj.name).

Hmm, can we require toString()? Then from() will call it by default. But then again, you'll still need the idToTimeZone…

Yes, that would be fine, implementing name would be optional then. I'll change this.

In #294 various ways of getting around the idToTimeZone requirement are suggested.

ptomato · 2020-04-27T17:51:41Z

@Ms2ger Although #294 is still under discussion, I've changed this to use the fromId() method that we had suggested. I believe the comments are resolved as much as possible without #294, are there any blockers left in your opinion?

Ms2ger · 2020-04-28T08:25:34Z

Let's merge it as a draft, we can iterate from there.

ptomato requested review from Ms2ger, pipobscure, jasonwilliams, sffc, gibson042 and ryzokuken April 6, 2020 18:52

sffc reviewed Apr 6, 2020

View reviewed changes

ptomato force-pushed the custom-time-zone-draft branch from fbf0b29 to 548402c Compare April 6, 2020 21:27

sffc reviewed Apr 6, 2020

View reviewed changes

ptomato force-pushed the custom-time-zone-draft branch 2 times, most recently from 7510f28 to 8bfd98d Compare April 6, 2020 21:52

sffc approved these changes Apr 6, 2020

View reviewed changes

Ms2ger reviewed Apr 9, 2020

View reviewed changes

ptomato force-pushed the custom-time-zone-draft branch from 8bfd98d to 713b46a Compare April 9, 2020 19:01

ptomato force-pushed the custom-time-zone-draft branch 2 times, most recently from ae419d6 to 3818fd9 Compare April 13, 2020 16:47

Ms2ger reviewed Apr 14, 2020

View reviewed changes

ptomato force-pushed the custom-time-zone-draft branch from 3818fd9 to 6522247 Compare April 15, 2020 16:04

ptomato mentioned this pull request Apr 16, 2020

Semantics of idToCalendar function in Temporal.Date.from() #294

Closed

ptomato force-pushed the custom-time-zone-draft branch from 6522247 to 3a6e65f Compare April 16, 2020 23:21

Draft of custom time zone API

c011110

ptomato force-pushed the custom-time-zone-draft branch from 3a6e65f to c011110 Compare April 27, 2020 17:47

ptomato mentioned this pull request Apr 27, 2020

Write cookbook of practical scenarios using Temporal #240

Closed

Ms2ger merged commit e3d232e into main Apr 28, 2020

Ms2ger deleted the custom-time-zone-draft branch April 28, 2020 08:25

littledan mentioned this pull request May 14, 2020

Provide a means for attenuating time zone data #273

Closed

littledan mentioned this pull request May 20, 2020

Design of Temporal.Calendar and Temporal.TimeZone classes #300

Closed

ptomato mentioned this pull request May 21, 2020

TimeZone.getPossibleAbsolutesFor() #589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft of custom time zone API #498

Draft of custom time zone API #498

ptomato commented Apr 6, 2020

codecov bot commented Apr 6, 2020 •

edited

Loading

sffc left a comment

sffc Apr 6, 2020

sffc Apr 6, 2020

ptomato Apr 6, 2020

sffc Apr 6, 2020

ptomato Apr 6, 2020

Ms2ger left a comment

Ms2ger Apr 9, 2020

ptomato Apr 9, 2020

Ms2ger Apr 9, 2020

ptomato Apr 9, 2020

Ms2ger Apr 9, 2020

ptomato Apr 9, 2020

ryzokuken commented Apr 10, 2020

Ms2ger Apr 14, 2020

ptomato Apr 14, 2020

Ms2ger Apr 14, 2020

ptomato Apr 14, 2020

Ms2ger Apr 15, 2020

ptomato Apr 15, 2020

ptomato commented Apr 27, 2020

Ms2ger commented Apr 28, 2020


		> FIXME: This means we have to remove any checks for the _[[InitializedTemporalTimeZone]]_ slot in any APIs outside of `Temporal.TimeZone`.

		> FIXME: `Temporal.TimeZone` is supposed to be an iterable through all time zones known to the implementation, but what do we do about custom time zones there?

		/** Given the calendar/wall-clock time, returns an array of 0, 1, or
		* 2 absolute instants that are possible points on the timeline

		Alternatively, a custom time zone doesn't have to be a subclass of `Temporal.TimeZone`.
		In this case, it can be a plain object, which should (must?) implement all of the non-static methods in the interface.


		These functions gain an `options` parameter with an `idToTimeZone` option, whose value is a function returning the appropriate time zone object for the `id` passed in, or `null` to indicate that the time zone doesn't exist.

		If the option is not given, `Temporal.TimeZone.idToTimeZone()` is called.

Draft of custom time zone API #498

Draft of custom time zone API #498

Conversation

ptomato commented Apr 6, 2020

codecov bot commented Apr 6, 2020 • edited Loading

Codecov Report

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ms2ger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Apr 27, 2020

Ms2ger commented Apr 28, 2020

codecov bot commented Apr 6, 2020 •

edited

Loading