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

Allow 'week' synonym for 'weeks' for smallestUnit/largestUnit/unit option values #1491

Closed
justingrant opened this issue Apr 16, 2021 · 12 comments · Fixed by #1509
Closed

Allow 'week' synonym for 'weeks' for smallestUnit/largestUnit/unit option values #1491

justingrant opened this issue Apr 16, 2021 · 12 comments · Fixed by #1509
Labels
documentation Additions to documentation needs plenary input Needs to be presented to the committee and feedback incorporated non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

Per #325 (comment), singular variations of unit names will be preferred as values for the smallestUnit/largestUnit/unit options in various Temporal methods, although plural variants are also allowed.

Temporal already accepts both singular and plural variants for almost all units, so this change is almost entirely a documentation change not a normative change. However, there is one problematic case: currently 'week' is not allowed because there is no week property on any Temporal type.

This issue tracks the spec and polyfill changes required to accept 'week' as well for smallestUnit/largestUnit/unit options. This is a non-breaking change for existing polyfill users and will be a simplification for implementers because the weeks unit will no longer need to be special-cased.

BTW, this issue came about from feedback from the team working on the DurationFormat proposal (see #325), because in 402, all unit name strings are always singular. The Temporal champions believe that aligning to this precedent to always use singular names for unit strings is a good idea, and that this alignment is worth this (very small and non-breaking) normative change to the Temporal spec.

We'll retain the ability to accept plural strings to allow more permissive input from users.

@ptomato ptomato added needs plenary input Needs to be presented to the committee and feedback incorporated documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Apr 16, 2021
@ptomato
Copy link
Collaborator

ptomato commented Apr 19, 2021

I thought of another normative change that would need to be made in order to accommodate this. In places where we internally create an options object and set a largestUnit or smallestUnit property on it, in order to pass to a calendar method which may be user code, we would need to create the property with the singular value instead of the plural value.

For example, step 9.d.iv of https://tc39.es/proposal-temporal/#sec-temporal-unbalancedurationrelative

@FrankYFTang
Copy link
Contributor

Any update? @ryzokuken is updating spec text for https://github.com/tc39/proposal-intl-duration-format/ now. It will be nice if Temporal team can make the normative change to accept 'week' for option reading so that spec can refer to the correct one here.

@justingrant
Copy link
Collaborator Author

justingrant commented May 6, 2021

@FrankYFTang, the Temporal champions agreed to make this change, and part of the work (docs and TS types) is already done in draft PRs but the polyfill and spec changes are still outstanding. Because it's a normative change, my understanding is that we'll need to get plenary approval before merging the changes.

@ptomato - Is the remaining work on this change on your radar (or another Igalian's) to complete before the plenary meeting later this month? And have we already put this on the plenary agenda?

@ptomato
Copy link
Collaborator

ptomato commented May 7, 2021

It's on my radar, but I'm not sure it's necessary to complete for this month's plenary. If I get around to it before the agenda deadline, I'll add it, but I don't think DurationFormat needs to block on this.

It may well be more convenient to 'save up' normative changes that we discover while writing test262 tests and browser implementations (such as #1508 and some of the items in #1502) and run them by the plenary all at once.

@justingrant
Copy link
Collaborator Author

My main concern about getting this change done soon (in the docs and TS types at least) is because people are starting to play with Temporal and to write Temporal-using code samples. We won't want to have outdated legacy code patterns published in blogs, Stack Overflow, etc.

The other open issues affect implementors but seem to have either no effect on public-facing API shapes, or only affect rarely-used APIs like getISOFields. If there are other changes which affect widely-used APIs, then I'd suggest front-loading those changes too.

ptomato added a commit that referenced this issue May 10, 2021
Both singular and plural names remain accepted everywhere, and 'week' is
now accepted where previously only 'weeks' was. However, the singular form
is now preferred for Temporal.Duration as well, and the spec text should
reflect this.

Ensure that options objects passed to user code have the correct singular
unit strings on them.

Closes: #1491
@ptomato
Copy link
Collaborator

ptomato commented May 10, 2021

I don't have a lot of time to work on Temporal this week. I did make a quick attempt at the spec text changes in #1509, but I won't have time to follow this up with the accompanying test262 tests, or build a slide deck, by Friday.

I suppose since it's only a consensus PR and not a stage advancement, it would be OK to add it to the agenda after the agenda deadline. I also don't know if the test262 tests would be required to get consensus from the plenary. Can someone confirm?

@ljharb
Copy link
Member

ljharb commented May 10, 2021

The deadline doesn't apply to anything but proposal advancement; tests aren't required for consensus but would likely be required to land it in the main spec (and possibly to land it in the temporal spec)

@justingrant
Copy link
Collaborator Author

OK how about this plan?

  1. In the upcoming May meeting, we ask for consensus on the handful of pending (all pretty minor) normative changes, including the one in this issue
  2. Assuming consensus, we'll check in changes to the polyfill and Temporal spec as soon as they're available. At the same time, we'll merge the docs and docs and TS changes.
  3. Test262 tests will be added corresponding to these changes before checking in temporal content into the main 262 spec
  4. @ptomato I can help with slides if needed, just let me know. I have time.

(and possibly to land it in the temporal spec)

How do we determine if this is actually a requirement or not? Given we're in Stage 3, I'm nervous about further delays to checking in developer-facing API changes (especially in the docs) because every month we wait, that's another month for incorrect legacy code samples to accumulate in Stack Overflow, in developer blogs, etc.

@ljharb
Copy link
Member

ljharb commented May 12, 2021

@justingrant i agree with your concerns. it's an explicit requirement for the main spec PR to be merged, and I don't think it's really a requirement at all for merging it here; it's more that unless a change is in the test262 tests, engines are highly likely to implement the tested behavior and not the spec behavior.

@ptomato
Copy link
Collaborator

ptomato commented May 12, 2021

I acknowledge that this is a concern, but I am just not able to continue long-term pulling the kind of effort that I did in the run-up to Stage 3. Here's what I can do this week:

It sounds like that will be sufficient to ask for consensus for these two items in the plenary, since we can write test262 tests afterwards. So the plan sounds good to me.

I don't think I'll be able to sort through all the items in #1502 in time, so unfortunately if any normative changes arise from that, they will need to wait for a following plenary.

@justingrant I would appreciate help with the slides. In fact, totally optional, but if you want to use your invited expert status to present it yourself, that would free me up some time next week to write tests and continue helping with #1449 😄

@justingrant
Copy link
Collaborator Author

I acknowledge that this is a concern, but I am just not able to continue long-term pulling the kind of effort that I did in the run-up to Stage 3.

Agree! No need to burn the midnight oil for this-- my concern is mostly about getting plenary approval now so we're not blocked to merge the changes once they're ready.

It sounds like that will be sufficient to ask for consensus for these two items in the plenary, since we can write test262 tests afterwards. So the plan sounds good to me.

Cool! Sounds good.

I would appreciate help with the slides. In fact, totally optional, but if you want to use your invited expert status to present it yourself, that would free me up some time next week to write tests and continue helping with #1449 😄

Sure, I'd be happy to build and present slides. Will be fun. Only gotcha will be timing/scheduling, because I have hard stops at 1:30PM PDT both days. What's the right process to get the slides into the agenda, and to request a pre-1:30 time?

Other than #1508 and #1509, is there any other content needed for slides? Do we want to provide an implementation update? Anything else?

Also, what portion of the meeting agenda does this go in? In the proposals section of https://github.com/tc39/agendas/blob/master/2021/05.md, in the "Short Discussions" section, or somewhere else?

@ptomato
Copy link
Collaborator

ptomato commented May 18, 2021

Justin wrote some slides in the meantime and the agenda item is here: tc39/agendas#992

ptomato added a commit that referenced this issue May 20, 2021
Both singular and plural names remain accepted everywhere, and 'week' is
now accepted where previously only 'weeks' was. However, the singular form
is now preferred for Temporal.Duration as well, and the spec text should
reflect this.

Ensure that options objects passed to user code have the correct singular
unit strings on them.

Closes: #1491
ptomato added a commit that referenced this issue May 21, 2021
Both singular and plural names remain accepted everywhere, and 'week' is
now accepted where previously only 'weeks' was. However, the singular form
is now preferred for Temporal.Duration as well, and the spec text should
reflect this.

Ensure that options objects passed to user code have the correct singular
unit strings on them.

Closes: #1491
ptomato added a commit that referenced this issue May 21, 2021
This is the implementation of the normative changes in #1509 in the
polyfill, accompanied by test262 tests to ensure that passing plural and
singular values for largestUnit, smallestUnit, and unit behaves the same;
and also that Calendar.dateUntil() is only ever called with options bags
that contain singular values for largestUnit.

See: #1491
ptomato added a commit that referenced this issue May 29, 2021
Both singular and plural names remain accepted everywhere, and 'week' is
now accepted where previously only 'weeks' was. However, the singular form
is now preferred for Temporal.Duration as well, and the spec text should
reflect this.

Ensure that options objects passed to user code have the correct singular
unit strings on them.

Closes: #1491
ptomato added a commit that referenced this issue May 29, 2021
This is the implementation of the normative changes in #1509 in the
polyfill, accompanied by test262 tests to ensure that passing plural and
singular values for largestUnit, smallestUnit, and unit behaves the same;
and also that Calendar.dateUntil() is only ever called with options bags
that contain singular values for largestUnit.

See: #1491
ptomato added a commit that referenced this issue May 31, 2021
Both singular and plural names remain accepted everywhere, and 'week' is
now accepted where previously only 'weeks' was. However, the singular form
is now preferred for Temporal.Duration as well, and the spec text should
reflect this.

Ensure that options objects passed to user code have the correct singular
unit strings on them.

Closes: #1491
ptomato added a commit that referenced this issue May 31, 2021
This is the implementation of the normative changes in #1509 in the
polyfill, accompanied by test262 tests to ensure that passing plural and
singular values for largestUnit, smallestUnit, and unit behaves the same;
and also that Calendar.dateUntil() is only ever called with options bags
that contain singular values for largestUnit.

See: #1491
Ms2ger pushed a commit that referenced this issue Jun 1, 2021
This is the implementation of the normative changes in #1509 in the
polyfill, accompanied by test262 tests to ensure that passing plural and
singular values for largestUnit, smallestUnit, and unit behaves the same;
and also that Calendar.dateUntil() is only ever called with options bags
that contain singular values for largestUnit.

See: #1491
ptomato added a commit that referenced this issue Jun 1, 2021
This is the implementation of the normative changes in #1509 in the
polyfill, accompanied by test262 tests to ensure that passing plural and
singular values for largestUnit, smallestUnit, and unit behaves the same;
and also that Calendar.dateUntil() is only ever called with options bags
that contain singular values for largestUnit.

See: #1491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation needs plenary input Needs to be presented to the committee and feedback incorporated non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants