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

Open
arshaw opened this issue Mar 1, 2024 · 6 comments
Open

Clarify timeStyle:long/full for Plain types #2795

arshaw opened this issue Mar 1, 2024 · 6 comments
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

No branches or pull requests

2 participants