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

@ptomato Stage 3 review #33

Closed
justingrant opened this issue Jun 20, 2023 · 5 comments
Closed

@ptomato Stage 3 review #33

justingrant opened this issue Jun 20, 2023 · 5 comments

Comments

@justingrant
Copy link
Collaborator

This is a tracking issue for @ptomato to review the Time Zone Canonicalization proposal spec at https://tc39.es/proposal-canonical-tz/#sec-canonical-tz-intro.

The spec text is easiest to review as a web page rather than the raw spec text, because most of the text is just existing AOs with only one line changed. The total scope of changes is <20 lines + one prose paragraph.

@ptomato
Copy link
Contributor

ptomato commented Jun 20, 2023

This is a positive review assuming the open issues that affect behaviour will be closed.

@justingrant
Copy link
Collaborator Author

Thanks Philip!

Agreed

There's one more TG2 meeting before the next plenary where I'll try to get consensus one more time on open issues in advance of the next TC39 plenary.

The most urgent changes in this proposal are the normative changes to stop canonicalizing before storing internal slots, and the new TimeZone.p.equals method. Anything else can be handled in later normative or editorial PRs. So my plan is to lock the normative scope of this proposal at the next plenary if the proposal is approved for Stage 3.

I'm not sure it makes sense to close the issues just yet, because if we do get implementer consensus during Stage 3 then I'd be open to editorial text in this proposal (e.g. "implementations are recommended to...") to reify that consensus, and it's helpful to have a honeypot location for those discussions. But I'm also OK to to migrate those issues to the 402 repo if that's better. Let me know what you'd prefer.

If we do leave those issues open in the proposal, then we'll mark them with a label and title showing that only editorial changes will be made in this proposal's text (e.g. recommendations for implementers), not anything normative.

The resolution of this issue seems unlikely to happen soon, and also unlikely to yield anything other than an editorial note in the SystemTimeZoneIdentifier spec text (e.g. "Many implementations rely on a mapping layer between the host environment's time zone and the IANA Time Zone Database. These implementations are recommended to..."). Note that this encompasses not just Windows zones, but also ICU behavior which overrides what the OS says.

This is a good example of an issue that can be ported to 402 either before or during Stage 3 if we can't get implementer consensus on good text. It should not block this proposal moving fwd.

  • Does the open issue about the waiting period (Concerns with waiting period for identifier renames #17) affect anything in stage 3? I expect not, since the answer comes from implementation experience, but I think resolving this is a requirement for stage 4. Can we remove the TODO in the spec text and just keep the issue open?

Nope. I'll remove the TODO. Should I add an editorial note pointing people at the open issue?

Thanks, will fix.

This is a positive review assuming the open issues that affect behaviour will be closed.

🎉

@ptomato
Copy link
Contributor

ptomato commented Jun 20, 2023

OK, notes to reify the consensus sounds good to me. I'll assume the upcoming TG2 meeting will resolve most or all of these. If there's no signals from TG2 that any of these resolutions would result in significant changes, then I think it's fine to move forward.

@justingrant
Copy link
Collaborator Author

FYI: Temporal just merged an editorial PR to prepare for tc39/proposal-temporal#2607, which is the normative PR that limits offset time zones to minute precision.

As a result, today a few links in the proposal-canonical-tz spec text were broken, and a few lines of the proposal spec needed updating. I just landed a PR that catches up this proposal's spec and polyfill with those upstream changes. Apologies for the churn!

If you've already reviewed the rest of proposal-canonical-tz, please take a look at SystemTimeZoneIdentifier that has 2 changed lines + a new editor's note, and TimeZoneEquals that has 5 newly-changed lines.

AFAIK there are no more changes expected (either here or upstream) before the Bergen meeting.

@justingrant
Copy link
Collaborator Author

This proposal reached Stage 3 at the July 2023 TC39 meeting. As decided in the meeting, the next step is to merge this proposal into Temporal Stage 3 via tc39/proposal-temporal#2633. Thanks for the reviews and for your support of this proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants