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

ECMA-262 editors' Stage 3 review #37

Closed
justingrant opened this issue Jun 22, 2023 · 9 comments
Closed

ECMA-262 editors' Stage 3 review #37

justingrant opened this issue Jun 22, 2023 · 9 comments

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jun 22, 2023

This is a tracking issue for @tc39/ecma262-editors to review the Time Zone Canonicalization proposal spec at https://tc39.es/proposal-canonical-tz/#sec-canonical-tz-intro. Feel free to add feedback via comments here or open new issues, whichever is easiest.

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 <15 lines of changes to AOs and one <emu-note>.

You may want to review the Stage 3 editors' feedback threads at #33, #34, and #35.

Tests can be reviewed at tc39/test262#3837.

Thanks!

@michaelficarra
Copy link
Member

The 262 changes LGTM.

@bakkot
Copy link
Contributor

bakkot commented Jun 28, 2023

Also LGTM though of course the relevant AOs (CanonicalizeTimeZoneOffsetString, FormatTimeZoneOffsetString) will need to be pulled in to 262 (out of the Temporal proposal) if this lands before Temporal.

@justingrant
Copy link
Collaborator Author

Thanks for the reviews!

the relevant AOs (CanonicalizeTimeZoneOffsetString, FormatTimeZoneOffsetString) will need to be pulled in to 262

Yep. Although we could also editorially refactor to use a parsing-only solution like in tc39/proposal-temporal@d6b7059 that could avoid pulling in these AOs.

I'm playing PR chicken with tc39/proposal-temporal#2607 which includes that commit, because limiting offset time zones to minutes precision necessarily requires enforcing minutes-only in SystemTimeZoneIdentifier. If that normative Temporal PR gets consensus in July (which I hope it will because implementers have been so vehement about making the offset-time-zones-minute-precision change), then it removes the need for any SystemTimeZoneIdentifier changes in this proposal and I'll remove them here because they won't be needed anymore. Which will also remove the need to pull in those AOs. 🤞

if this lands before Temporal.

This raises an interesting process question that's related to this upcoming Stage 3 slide: if this proposal makes it to Stage 3, then how should it relate to Temporal?

There are two questions:

  1. Is it worth merging the non-Temporal-dependent parts of this proposal into 262 and 402 if this proposal reaches Stage 4 before Temporal does? Choices:

    • a) Yes, landing Temporal will take a long time so we should split out the non-Temporal-dependent parts and land them first.
    • b) No, implementers already have their hands full implementing Temporal, and doing more work in the same parts of the code seems like lots of hassle for at most a few months of benefit. Temporal is already making 262 and 402 changes to the exact same AOs, so let's just integrate those changes into Temporal and save everyone's time while we focus on landing Temporal.
    • c) Wait and see, decide this when this proposal is closer to Stage 4. There's no rush to decide this before there's any implementations done, and if Temporal reaches Stage 4 first then it doesn't matter anyways.
  2. If this proposal reaches Stage 3, how should the Temporal-dependent parts of this proposal be implemented?

    • a) Wait until Temporal is implemented, then implement this proposal on top
    • b) Merge this proposal into Temporal Stage 3, so that implementers won't have to re-do work and to reduce the risk of waiting too long after Temporal is implemented before this proposal is rolled out (because Temporal makes the problems worse that this proposal is solving).
    • c) Something else?

Do you have a preferred answer for either or both questions?

They're somewhat related, i.e. 1b may imply 2b or vice versa. A common theme is that it's unclear how to handle a smaller, faster proposal that's stacked on a larger, slower proposal. Once the hare catches up to the tortoise, should the hare climb on top of or walk slowly behind the tortoise?

Personally, I like 1b or 1c but happy to follow whatever others want to do. I prefer 2b (and I suspect implementers will prefer that too) but need to get feedback from the rest of the Temporal champions about that.

My only strong opinion is that I'm concerned that 2a might delay implementation too long (especially if implementers don't want to prioritize any date/time related work for a long while after Temporal lands), because a goal of this proposal is to get it landed before Temporal is widely adopted.

I'm curious to hear what you think about these questions!

@ljharb
Copy link
Member

ljharb commented Jun 29, 2023

This one doesn’t seem like it can advance beyond temporal, meaning that if it’s still at stage 3 then this one should remain there. Is that accurate?

@justingrant
Copy link
Collaborator Author

Depends on (1) above. If we want to land the non-Temporal dependent parts of this proposal (namely the changes to Intl.DateTimeFormat.p.resolvedOptions().timeZone) before Temporal hits Stage 4, then we could do so. Not sure that would add a lot of value, but it'd be possible.

But I think the easiest decision would be to do what you recommend: snap this proposal to Temporal's schedule so that implementers only have one thing to worry about building and testing.

It's an interesting question exactly what "snap this proposal to Temporal's schedule" means exactly. If this proposal reaches Stage 3, should we merge it into Temporal's spec text so that implementers won't have to connect the dots themselves? Or best to leave them separate?

@justingrant
Copy link
Collaborator Author

Also, 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

@tc39/ecma262-editors - if you haven't already weighed in above, do you have any concerns with the spec text of this proposal, or any other part of its Stage 3 readiness? Thanks!

@syg
Copy link

syg commented Jul 10, 2023

The 262 changes also LGTM.

@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

5 participants