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

Cookbook example for compiling time zone from tzdata rules #1370

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Feb 19, 2021

An example of parsing the rules from the tzdata rules (sometimes called "Olson database") directly, and creating a Temporal.TimeZone object out of them. By far the longest cookbook example.

A few additional bug fixes in the polyfill, as well.

Closes: #605

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.36%. Comparing base (d51828d) to head (4fc9cf5).
Report is 2179 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1370   +/-   ##
=======================================
  Coverage   95.36%   95.36%           
=======================================
  Files          19       19           
  Lines       11123    11123           
  Branches     1826     1826           
=======================================
  Hits        10607    10607           
  Misses        511      511           
  Partials        5        5           
Flag Coverage Δ
test262 48.45% <0.00%> (ø)
tests 91.06% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptomato
Copy link
Collaborator Author

ptomato commented Feb 19, 2021

Huh, I thought we had private fields in Node 12, but I guess it was literally only fields and not methods. I'll rewrite the example not to use them.

@ljharb
Copy link
Member

ljharb commented Feb 19, 2021

Private methods and accessors only landed in node 14 something.

Copy link
Collaborator

@cjtenny cjtenny left a comment

Choose a reason for hiding this comment

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

Pending moving the ES.TemporalDurationToString fixes out from here per #1375, lgtm! (only separate them if you still want to - I'm fine shipping them as part of this).

I think it could be nice adding a quick test case for Pacific/Apia's day skip as well since we reference that example elsewhere in the docs a lot, but it's fine without it too.


An example of building your own `Temporal.TimeZone` object from [tzdata-compatible rules](https://data.iana.org/time-zones/tz-how-to.html).

→ [Time zone from tzdata](cookbook-tzdata.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to this is split into a separate md file? The other examples all embed directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two reasons: one is that these last few examples are much larger than the other cookbook snippets, so for practicality they can live on their own pages. The other reason is that these last few examples are not so much intended as "copy this recipe if you want to do X" but more like "here's something esoteric that Temporal is capable of, that you probably won't need to do, but you might adapt this recipe for something else".

@gilmoreorless
Copy link
Contributor

Is this intended to be a general-purpose tzdata parser, or just the minimum needed for the America/Chicago example? I only ask because I can see it will fail for some other zones' source data. But I wanted to know the purpose before digging into specifics.

equal() was forgotten here, leading to these tests always passing.
These tests called getNextTransition() and getPreviousTransition() four
times on the same instant, instead of chaining four calls.
getPreviousTransition() returns the last transition before the instant
passed into it, not the last transition before or equal to. Otherwise,
you would have to manually subtract a nanosecond in between each call if
you wanted to get a series of previous transitions.

Add similar tests for getNextTransition(), although that wasn't broken.

No change needed to the spec text or documentation, which are already
correct.
An example of parsing the rules from the tzdata (sometimes called "Olson
database") directly, and creating a Temporal.TimeZone object out of them.

Closes: #605
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 22, 2021

I think it could be nice adding a quick test case for Pacific/Apia's day skip as well since we reference that example elsewhere in the docs a lot, but it's fine without it too.

This is done, and it uncovered several bugs. So thanks for the suggestion. For ease of review, here's a list of what I had to change:

  • added Pacific/Apia tzdata records, and tests for them
  • fixed bug in getRuleSetInEffect() not calculating the correct instant in which the state ends (added standard offset instead of subtracting)
  • started cacheing dstShift in the cache records as well, because it's necessary when considering the first DST rule change during a calendar year (previously I erroneously assumed it to be 0)
  • if the state ends before the DST rule ends, cache the state ending time instead of the DST rule ending time
  • don't fall through to next year's DST transitions if the state ends this year

Is this intended to be a general-purpose tzdata parser, or just the minimum needed for the America/Chicago example? I only ask because I can see it will fail for some other zones' source data. But I wanted to know the purpose before digging into specifics.

I wouldn't go so far as to claim that it's general purpose; that said, what does it fail on? I'd like to investigate that, even if not now then some other time. I have actually measured getNextTransition()/getPreviousTransition() to be faster using this code than the polyfill's current approach of using Intl to get the offset and bisecting, so it might come in handy when working on a production-quality polyfill.

@ptomato ptomato requested a review from cjtenny February 22, 2021 23:00
@gilmoreorless
Copy link
Contributor

I wouldn't go so far as to claim that it's general purpose; that said, what does it fail on? I'd like to investigate that, even if not now then some other time.

I tried it with the Europe/Dublin data, knowing that that zone has caused problems for other parsers recently (e.g. Java's tzupdater broke when "negative DST" was introduced). It looks like some of the bugs I saw have been fixed by your latest changes.

A couple of things still remain, though. The RULES column of a Zone definition can contain a UTC offset instead of a named Rule. (The Pacific/Honolulu example at the bottom of the referenced how-to file also shows this case).

Zone	Europe/Dublin	-0:25:00 -	LMT	1880 Aug  2
			-0:25:21 -	DMT	1916 May 21  2:00s
			-0:25:21 1:00	IST	1916 Oct  1  2:00s
			 0:00	GB-Eire	%s	1921 Dec  6 # independence

The third line has a rule of 1:00 as a shortcut for "add one hour to the standard offset", which isn't currently handled. The code throws TypeError: ruleSet is not iterable.

There's also a subtle bug lurking in that example above. The s or u suffix on the "until" time is handled when parsing a Rule, but not a Zone. When the time ends with :00s, the code still works because 00s gets coerced to NaN during parsing, but that then falls back to the default 0 when being passed into PlainDateTime. But when the suffix is u, the behaviour is incorrect. A good test for this is Atlantic/Azores based on its second line:

Zone Atlantic/Azores	-1:42:40 -	LMT	1884        # Ponta Delgada
			-1:54:32 -	HMT	1912 Jan  1  2:00u # Horta MT

I have actually measured getNextTransition()/getPreviousTransition() to be faster using this code than the polyfill's current approach of using Intl to get the offset and bisecting, so it might come in handy when working on a production-quality polyfill.

Agreed on this. I noticed the difference when stepping through the code in a debugger while trying to pinpoint the case above. This new code is definitely faster.

Copy link
Collaborator

@cjtenny cjtenny left a comment

Choose a reason for hiding this comment

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

Sorry for forgetting about this one; I didn't previously re-review because of @gilmoreorless' assertion that some bugs remain, but then I forgot about it for quite a while as a result. If it's not intended to be general and just an example that works only with the included data, then I'm still good with shipping it! No new comments on the changes since prior review, still looks good to me.

@ptomato
Copy link
Collaborator Author

ptomato commented Apr 8, 2021

Oh, I am still planning to address those, but it's on a lower priority right now until we get all the changes from the March TC39 plenary landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookbook example of custom time zone from tzdata rules
5 participants