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

Should locales.timeZone throw in Temporal.DateTime.prototype.toLocaleString() ? #577

Closed
justingrant opened this issue May 16, 2020 · 7 comments · Fixed by #958
Closed

Should locales.timeZone throw in Temporal.DateTime.prototype.toLocaleString() ? #577

justingrant opened this issue May 16, 2020 · 7 comments · Fixed by #958
Assignees
Labels
documentation Additions to documentation

Comments

@justingrant
Copy link
Collaborator

justingrant commented May 16, 2020

https://tc39.es/proposal-temporal/docs/datetime.html#toLocaleString

NOTE: Unlike in Temporal.Absolute.prototype.toLocaleString(), locales.timeZone will have no effect, because Temporal.DateTime carries no time zone information and is just a wall-clock time.

Why not throw if this option is used? If we don't throw, then a naive developer will run the code, see the "correct" time zone (the local environment's) and assume that the code is working fine.

I'm not suggesting throwing if the default format includes a time zone, only when the user manually requests a time zone in the format options. (#572 covers the default format case)

If this does throw an exception, then would be cool if the error message suggested that the developer use Temporal.Absolute.prototype.toLocaleString() instead.

@ryzokuken ryzokuken self-assigned this May 18, 2020
@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels May 18, 2020
@ryzokuken ryzokuken added this to the Stable API milestone May 18, 2020
@ryzokuken
Copy link
Member

I guess the question of "should we throw" could be discussed by the champions group in greater detail, but I guess one bug I could spot was that it should say options.timeZone instead of locales.timeZone. I'll make a PR to fix that. Thanks, @justingrant.

@ptomato
Copy link
Collaborator

ptomato commented May 18, 2020

I'd be in favour of not throwing, but also definitely not displaying a time zone (which I'll fix in #572).

That would match what we do with other types when requesting a format for a unit that they don't have:

> Temporal.now.time().toLocaleString('en', { month: 'long' })
'11:59:37 a.m.'

And also the behaviour of legacy Date:

> new Date().toLocaleString('en', { millennium: 'numeric' })
'2020-05-18, 12:03:04 p.m.'

@justingrant
Copy link
Collaborator Author

I'd be in favour of not throwing

@ptomato - Sounds good to me. It might be nice to clarify the docs a little. Currently:

NOTE: Unlike in Temporal.Absolute.prototype.toLocaleString(), options.timeZone will have no effect, because Temporal.DateTime carries no time zone information and is just a wall-clock time.

Absolute doesn't know about a time zone either, so this justification seems kinda inaccurate. The real problem is ambiguity: a DateTime can't know whether it's localized time zone name if Pacific Daylight Time or Pacific Standard Time during the hour before/after DST ends.

How about something like this?

NOTE: options.timeZone will have no effect, because Temporal.DateTime carries no time zone information. Also, some localized time zone names like "Pacific Daylight Time" vs. "Pacific Standard Time" cannot be uniquely determined from a DateTime during during the hour before or after Daylight Savings Time ends. Therefore, to display a localized date and time including its time zone, use Temporal.Absolute.prototype.toLocaleString().

@ptomato
Copy link
Collaborator

ptomato commented May 18, 2020

I'm not sure I agree, I still believe the justification is correct. Even though Absolute doesn't carry a time zone, it is a known point in time, which can be expressed in a time zone, so we allow formatting it with a time zone. The reason DateTime should never print with a time zone is not because the time zone is ambiguous during DST changes. DateTime just doesn't have a time zone, and I think it'd be misleading to suggest that it has some sort of "hidden" one that we don't expose because it might be ambiguous.

@justingrant
Copy link
Collaborator Author

I guess what confused me was that Absolute doesn't have a time zone either. In both cases, to get a localized format that includes a time zone, the user needs to manually pass a time zone parameter.

So a reasonable user might ask "Hey, if I have to add a time zone parameter anyways, why are you making me round-trip over to Absolute (and specify the time zone yet again!)" if I already have a DateTime in the correct time zone and all I need is the time zone name to display at the end?".

One answer is conceptual & educational: we want to help users learn that DateTime doesn't have a timezone, so hiding it here helps users learn that DateTime doesn't have any time zone awareness (hidden or otherwise).

Another answer is functional: a DateTime cannot accurately display the localized time zone name because the localized time zone name requires knowing the time zone offset (e.g. EST vs. EDT) but the time zone offset of a DateTime cannot be unambiguously determined because it's ambiguous for the hour before/after DST ends.

For me, the second reason is more compelling but I think both reasons might resonate depending on the user.

Not a big deal either way though. The high-order bit, IMHO, is letting the user know that if they want localized {time, date, timezone} formats, they'll need to go over to Absolute to get it. I think the docs could be a little more clear on that point, although they're probably OK as-is.

@ptomato
Copy link
Collaborator

ptomato commented May 19, 2020

OK, l think the action item here is to see if we can make the documentation clearer. If you want to take a crack at it, feel free, otherwise I'll get around to it at some point.

I finally realized that this whole time, by ambiguity between EST/EDT, you've been referring to the time zone that you (implicitly) pass in via the toLocaleString options. With that in mind, I understand the wording you originally suggested, and I think it could work as long as we make that clearer.

@ryzokuken ryzokuken removed this from the Stable API milestone May 20, 2020
@sffc
Copy link
Collaborator

sffc commented May 20, 2020

Related (possible duplicate): #572

@sffc sffc added this to the Stage 3 milestone May 20, 2020
@ptomato ptomato modified the milestones: Stage 3, Stable API Jun 4, 2020
@ryzokuken ryzokuken removed their assignment Jun 10, 2020
@ptomato ptomato added documentation Additions to documentation and removed behavior Relating to behavior defined in the proposal feedback non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Aug 25, 2020
@ptomato ptomato added spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Sep 18, 2020
@ptomato ptomato removed non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 2, 2020
@ptomato ptomato self-assigned this Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants