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

Clarify timeStyle:long/full for Plain types #2795

Closed
arshaw opened this issue Mar 1, 2024 · 11 comments
Closed

Clarify timeStyle:long/full for Plain types #2795

arshaw opened this issue Mar 1, 2024 · 11 comments
Assignees
Labels
editorial non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! test262
Milestone

Comments

@arshaw
Copy link
Contributor

arshaw commented Mar 1, 2024

Hello!

All the Plain* types accept timeZoneName/timeZone for their toLocaleString string methods but they're ignored:

const pdt = Temporal.PlainDateTime.from({
  year: 2025,
  month: 1,
  day: 1,
  hour: 12,
  minute: 13,
})

pdt.toLocaleString('en', {
  hours: '2-digit',
  minutes: '2-digit',
  seconds: '2-digit',
  timeZoneName: 'long',
  timeZone: 'America/Chicago',
})
// '1/1/2025, 12:13:00 PM'

However, when timeStyle is set to either 'full' or 'long', the time zone is included:

pdt.toLocaleString('en', {
  timeStyle: 'full',
  timeZone: 'America/Chicago',
})
// '12:13:00 PM Central Standard Time'

I this intentional? Or just a shortcoming of the reference polyfill?

If it is in fact a shortcoming, and the reference polyfill should not be doing this, I would recommend internally forcing timeStyle 'full' or 'long' to 'medium' to omit the timeZone.

I've tested to see whether the 'medium' is always simply the 'long' or 'full' minus the timezone, and it seems to be the case:

https://codepen.io/arshaw/pen/MWRYKqV?editors=0010

This technique doesn't work for Dzongkha/Esperanto in Firefox/Safari, but still probably okay to force it 'medium' for those locales nonetheless.

I'm happy to create a PR for this if applicable.

@ptomato
Copy link
Collaborator

ptomato commented Mar 1, 2024

This is a shortcoming in the reference code, I think. Time zone should never be printed for Plain types. But I can't remember off the top of my head if the spec says to throw in that case, or to ignore the timeZone and timeZoneName.

@ptomato
Copy link
Collaborator

ptomato commented Mar 5, 2024

I looked at the spec and I believe it should be ignored.

In step 57 of CreateDateTimeFormat we consult Table 27 to see which options are supported for each Temporal type, and only copy supported options from formatOptions to limitedOptions for use in determining the DTF pattern for that Temporal type.

@arshaw
Copy link
Contributor Author

arshaw commented Mar 5, 2024

Thanks for the info @ptomato. The algorithm you describe for copying DateTimeFormat options makes sense.

The problem I'm posing is actually not very related to timeZone or timeZoneName. I was merely including timeZoneName in the first code example to prove how it IS ignored in the normal case. In the second code example, I was including timeZone in order to show a consistent reproduction result that didn't depend on the current user's locale. I probably should have made my code samples more minimal.

The crux of the problem is related to timeStyle. If you run the following code in the playground, you'll see how the time zone is displayed:

Temporal.Now.plainDateTimeISO().toLocaleString('en', {
  timeStyle: 'full',
})
// "3:14:26 PM Eastern Standard Time" for me

This is a shortcoming with the reference implementation specifically. The timeStyle:full/long options comes with the time zone baked in. My solution is to massage all timeStyle:full/long values to medium, which never shows the timezone. I'm happy to make a PR to the reference implementation if you feel this is a satisfactory workaround.

@ptomato
Copy link
Collaborator

ptomato commented Mar 6, 2024

Yes, please feel free! The reference polyfill already uses plenty of other workarounds in toLocaleString(). Unlike the rest of the code, toLocaleString() isn't written exactly like the spec, because we wanted to use the native DateTimeFormat objects as much as possible and not have to reimplement them.

@arshaw
Copy link
Contributor Author

arshaw commented Mar 7, 2024

Got it, thanks @ptomato.

Then I assume this is a mistake in this test:

https://github.com/tc39/test262/blob/46fc2814306c75f671924c6830346bf76b3ac30b/test/staging/Intl402/Temporal/old/datetime-toLocaleString.js#L50

The timeZone option should be ignored. And definitely shouldn't subject a PlainDateTime to a DST shift, right?

That test is currently being ignored but I can make a fix so that when it comes back online it won't cause problems.

@ptomato
Copy link
Collaborator

ptomato commented Mar 7, 2024

It's not obvious to me that there's a desired behaviour that will meet everyone's expectations all the time. But reading the spec I agree that timeZone should be ignored there. So the test seems wrong. Good catch!

@ptomato ptomato added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! no-spec-text PR can be ignored by implementors labels Mar 28, 2024
@anba
Copy link
Contributor

anba commented Jun 24, 2024

I've created #2904 to sync the Intl.DateTimeFormat changes with latest ECMA-402. Hopefully #2904 also makes it a bit easier to understand how the date-time format selection works. For this specific case (with and without #2904):

Temporal.Now.plainDateTimeISO().toLocaleString("en", {
  timeStyle: "full",
})

the same time format is used for all types (Temporal.PlainTime, Temporal.PlainDateTime, Temporal.ZonedDateTime, Temporal.Instant). This means time zone information is included, even for plain Temporal types.

Without #2904, this step is relevant:

d. If bestFormat does not have any fields that are in fields, then
  i. Set bestFormat to null.

When the current type is Temporal.PlainTime, then bestFormat and fields have a non-empty intersection, because both include "hour", "minute", and "second", so bestFormat is not set to null.

With #2904, the time format selection should be more easy to understand when timeStyle is used, because there's only a single call to DateTimeStyleFormat.


That being said, I'm not sure the current spec matches previous consensus. For example the current spec requires that this throw a TypeError in HandleDateTimeTemporalTime:

new Intl.DateTimeFormat("en", {dateStyle: "long"}).format(new Temporal.PlainTime());

whereas this call succeeds and returns the string "12:00:00 AM":

new Intl.DateTimeFormat("en", {day: "numeric"}).format(new Temporal.PlainTime());

@ptomato
Copy link
Collaborator

ptomato commented Sep 19, 2024

Meeting, 2024-09-19: We believe this is probably just a bug in the JS code and that the spec text is correct, but that needs to be verified.

The principle is that Intl.DateTimeFormat resolves options for all fields, but if you format a limited type like PlainYearMonth, you only use the options for year and month.

Just to make sure we are all talking about the same thing, I made a table of expected results (in my locale and time zone) for formatting the various limited Temporal types with various DateTimeFormat options. Let me know if you disagree with anything in this table.

format options Instant PDT PlainDate PYM PMD PlainTime
{dateStyle: 'short'} 2024-09-19 2024-09-19 2024-09-19 2024-09 09-19 throw
{day: 'numeric'} 19 19 19 throw 19 throw
{timeStyle: 'long'} 12:12:00 p.m. PDT 12:12:00 p.m. throw throw throw 12:12:00 p.m.
{hour: '2-digit'} 12 p.m. 12 p.m. throw throw throw 12 p.m.

Note that the time zone name is printed when formatting Instant with a timeStyle that includes the time zone. This is intentional, even though Instant does not include a time zone, it is the Temporal analogue to legacy Date and behaves the same way in formatting.

If there are no objections to this table, I'll verify that this is actually what the implementation-defined parts of the spec text suggest, and that it is covered by test262 tests.

@ptomato ptomato added test262 and removed no-spec-text PR can be ignored by implementors labels Sep 19, 2024
@anba
Copy link
Contributor

anba commented Sep 20, 2024

Let me know if you disagree with anything in this table.

The table doesn't match the specification. This is the case with or without #2904. In 763acd2, I wrote:

The default options processing in GetDateTimeFormat matches the previous
Temporal draft, but it's not clear at this point if this implements previous
concensus, because it means the unsupported format case can only apply when
dateStyle or timeStyle are used. IOW new Intl.DateTimeFormat("en", {day: "numeric"})
works with all Temporal types, including Temporal.PlainTime, whereas
new Intl.DateTimeFormat("en", {dateStyle: "short"}) throws when used with
Temporal.PlainTime.

@ptomato
Copy link
Collaborator

ptomato commented Sep 20, 2024

@anba, Sorry for not being clearer. You said the specification doesn't match what the consensus is, so I tried to make the table reflect the consensus.

@ptomato ptomato added this to the Stage "3.5" milestone Sep 20, 2024
@justingrant
Copy link
Collaborator

justingrant commented Oct 3, 2024

Meeting 2024-10-03: Throw if the Temporal type's data model has no overlap with options provided. Otherwise just ignore the option.

For example, if the only option is {day: "2-digit", hour: "2-digit"} and the type is Temporal.PlainMonthDay, then we'd ignore the hour option and use the day option. However, if the type is Temporal.PlainYearMonth, it'd have no overlap with the options so we'd throw.

@ptomato ptomato self-assigned this Oct 3, 2024
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
These staging tests are incorrect. See tc39/proposal-temporal#2795. This
was an unintended behaviour. It differed from the behaviour for dateStyle
and timeStyle, which was the intended behaviour.
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
…s and Temporal object

See tc39/proposal-temporal#2795. When attempting to format a Temporal
object, if the DateTimeFormat has options that do not overlap with the
data model of the Temporal object, toLocaleString() and format() are
supposed to throw a TypeError.
ptomato added a commit that referenced this issue Oct 4, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
These staging tests are incorrect. See tc39/proposal-temporal#2795. This
was an unintended behaviour. It differed from the behaviour for dateStyle
and timeStyle, which was the intended behaviour.
ptomato added a commit to ptomato/test262 that referenced this issue Oct 4, 2024
…s and Temporal object

See tc39/proposal-temporal#2795. When attempting to format a Temporal
object, if the DateTimeFormat has options that do not overlap with the
data model of the Temporal object, toLocaleString() and format() are
supposed to throw a TypeError.
ptomato added a commit that referenced this issue Oct 4, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit that referenced this issue Oct 4, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
Ms2ger pushed a commit to ptomato/test262 that referenced this issue Oct 7, 2024
These staging tests are incorrect. See tc39/proposal-temporal#2795. This
was an unintended behaviour. It differed from the behaviour for dateStyle
and timeStyle, which was the intended behaviour.
Ms2ger pushed a commit to ptomato/test262 that referenced this issue Oct 7, 2024
…s and Temporal object

See tc39/proposal-temporal#2795. When attempting to format a Temporal
object, if the DateTimeFormat has options that do not overlap with the
data model of the Temporal object, toLocaleString() and format() are
supposed to throw a TypeError.
Ms2ger pushed a commit to tc39/test262 that referenced this issue Oct 7, 2024
These staging tests are incorrect. See tc39/proposal-temporal#2795. This
was an unintended behaviour. It differed from the behaviour for dateStyle
and timeStyle, which was the intended behaviour.
Ms2ger pushed a commit to tc39/test262 that referenced this issue Oct 7, 2024
…s and Temporal object

See tc39/proposal-temporal#2795. When attempting to format a Temporal
object, if the DateTimeFormat has options that do not overlap with the
data model of the Temporal object, toLocaleString() and format() are
supposed to throw a TypeError.
ptomato added a commit that referenced this issue Oct 7, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
ptomato added a commit that referenced this issue Oct 7, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
ptomato added a commit that referenced this issue Oct 7, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit that referenced this issue Oct 7, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
ptomato added a commit that referenced this issue Oct 7, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
ptomato added a commit that referenced this issue Oct 7, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit that referenced this issue Oct 8, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
ptomato added a commit that referenced this issue Oct 8, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
Somewhere along the line the spec text got out of sync with the consensus,
probably in one of the rebases on newer ECMA-402. When attempting to
format a Temporal object, if the DateTimeFormat has options that do not
overlap with the data model of the Temporal object, toLocaleString() and
format() are supposed to throw a TypeError.

Previously, this worked correctly with dateStyle and timeStyle, but not
with other options. We would treat the options as if the DateTimeFormat
had been created with only default options, which was incorrect.

Closes: #2795
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.

See: #2795
@Ms2ger Ms2ger closed this as completed in c982717 Oct 8, 2024
Ms2ger pushed a commit that referenced this issue Oct 8, 2024
… types

timeStyle: 'long' and timeStyle: 'full' include the time zone name for
legacy Date formatting. However, PlainTime and PlainDateTime don't include
a time zone in their data model, so they shouldn't format a time zone name
when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay
shouldn't format a day or year respectively when using dateStyle. This was
already the consensus but somewhere along the line the spec text got out
of sync, probably in a rebase on latest ECMA-402.

In the polyfill, we already handled dateStyle for PlainYearMonth and
PlainMonthDay. Try to hack around this the same way for timeStyle.

Closes: #2795
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! test262
Projects
None yet
Development

No branches or pull requests

4 participants