Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Idea about combining dateStyle/timeStyle with other options #40

Closed
sffc opened this issue Nov 17, 2019 · 14 comments · Fixed by #43
Closed

Idea about combining dateStyle/timeStyle with other options #40

sffc opened this issue Nov 17, 2019 · 14 comments · Fixed by #43

Comments

@sffc
Copy link

sffc commented Nov 17, 2019

We decided in #2 to ignore superfluous options when dateStyle/timeStyle was being used.

However, I had an idea about a reasonable behavior for combining the two, which also allows us to cover additional use cases. I'll go them one by one.

Omitting undesired fields

By default, all fields of the date and all fields of the time are shown. However, we have users at Google who may like to use dateStyle/timeStyle but show only a certain subset of fields. One way to allow this would be to allow users to "opt out" of fields when using dateStyle/timeStyle.

Suppose the user wrote code such as,

new Date().toLocaleString(undefined, {
    dateStyle: "short",
    year: false
});

The expected output would then be a string with month and day but not year, formatted with the locale customs for short dates.

Overriding field widths

This is probably not as important of a use case, but it extends naturally from above. Suppose a user were to write,

new Date().toLocaleString(undefined, {
    timeStyle: "short",
    dayPeriod: "long"
});

The expected output here would be the appropriate long form for am/pm, but with the hour and minute (and second?) still shown in short form as is expected with timeStyle "short".

Implementation

There are workarounds to implement these features in ICU (basically round-tripping from pattern to skeleton back to pattern), but they are not as efficient as they could be. I have an upstream ticket where I am suggesting that the way dateStyle/timeStyle patterns are encoded into CLDR could be improved to allow the above cases: essentially, make dateStyle/timeStyle map to skeletons directly within CLDR (in a locale-dependent manner).

https://unicode-org.atlassian.net/browse/ICU-20710

I also filed a CLDR issue:

https://unicode-org.atlassian.net/browse/CLDR-13425

(Sorry to be raising this issue at Stage 3, but Anba's bug from implementing this in Firefox led to my line of thinking that made me raise this issue.)

@sffc
Copy link
Author

sffc commented Nov 17, 2019

@anba
Copy link
Contributor

anba commented Dec 4, 2019

There are workarounds to implement these features in ICU (basically round-tripping from pattern to skeleton back to pattern), but they are not as efficient as they could be.

https://unicode-org.atlassian.net/browse/ICU-13518 makes round-tripping error-prone, because the wrong skeleton may be returned for a given pattern.

@sffc
Copy link
Author

sffc commented Dec 12, 2019

I also posted some thoughts in #2; my fault for double-threading. Let's keep conversation here.

I said:

The main argument I had made in favor of ignoring unused options was the precedent of ignoring unused options in Intl.NumberFormat. However, the situation is a bit different when it comes to user expectations. A user would have less of a reasonable expectation for "currencyDisplay" to do anything when style is set to "unit". In dateStyle/timeStyle, though, I can see users legitimately trying to use the other options to override certain fields in dateStyle/timeStyle.

Examples:

// OK: obviously, currencyDisplay won't do anything here.
new Intl.NumberFormat(undefined, {
  style: "unit",
  unit: "meter",
  currencyDisplay: "narrow"
});

// Hmm, a user could have legitimate expectations for the following.
new Intl.DateTimeFormat(undefined, {
  dateStyle: "short",
  month: "long"
});

Another advantage of throwing would be that if we did decide to add interop with the other options in the future, such as suggested in #40, we can do that only if we throw today. If we ignore today, we have a higher risk of breaking the web.

@zbraniecki replied:

I don't have enough insight to claim which one is more likely to have an impact on the outcome, so my initial thought would be to postpone it till v2 and see if once we ship dateStyle/timeStyle people come to us with use cases.

IIRC we decided at the time to make an exception for hourCycle because it's a such a common matter of preference that most date/time formatting UIes provide a separate widget for it.

So, we wanted to have this to work:

new Intl.DateTimeFormat("en-US", {
  timeStyle: "short",
  hourCycle: "h24"
});

For other fields, my thoughts are:

  1. It could be a nice shortcut to allow for per-locale skeleton, but with adjustments
  2. It could be a papercut for developers as they may build a weird date-time formats while working with dateStyle/timeStyle.

I don't have enough insight to claim which one is more likely to have an impact on the outcome, so my initial thought would be to postpone it till v2 and see if once we ship dateStyle/timeStyle people come to us with use cases.

@sffc
Copy link
Author

sffc commented Dec 12, 2019

To summarize my thoughts: because a programmer may reasonably expect the combination of dateStyle/timeStyle with other options to have sensible behavior, and because it's not clear what that behavior should be, we should consider throwing RangeError when the options are combined. This gives us more time to think about what we want the behavior to be.

Throwing a RangeError in this situation would be a normative change, and would roll back what we had previously discussed in #2.

@zbraniecki
Copy link
Member

I wrote a PR that does not make an exception for hourCycle. I'd be in favor of making an exception for hourCycle. Reasons:

  1. This is not technically a field, it's a style on how to display a given field (hour)
  2. It is by far the most common user preference people chose and expect to be able to override
  3. We have a well defined algorithm already to update any skeleton to use customized hourCycle.

@sffc
Copy link
Author

sffc commented Jan 10, 2020

SGTM

@zbraniecki
Copy link
Member

I updated the PR to allow for hourCycle.

anba added a commit to anba/proposal-intl-datetime-style that referenced this issue Mar 31, 2020
… options

Drive-by change:
- Use standard EcmaSpeak when testing if both date- and timeStyle are undefined.

Fixes tc39#40.

Supersedes tc39#41.
@FrankYFTang
Copy link
Contributor

Test need to be changed in test262 tc39/test262#2659
We probably need more tests for this change
test/intl402/DateTimeFormat/constructor-options-order-timedate-style.js
test/intl402/DateTimeFormat/prototype/resolvedOptions/order-style.js

@FrankYFTang
Copy link
Contributor

Also, are there tests to ensure the TypeError do got throw when this happen in test262?

@ljharb
Copy link
Member

ljharb commented Sep 9, 2020

For posterity, I’ve received numerous reports of Chrome with this change breaking people’s code (passing fooStyle alongside other options was a noop before; now it throws)

@FrankYFTang
Copy link
Contributor

yea, I got https://bugs.chromium.org/p/chromium/issues/detail?id=1124778 today and I marked it as WaI

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Sep 9, 2020

@ljharb could you point out where are these reports ? I cannot find them in https://bugs.chromium.org/ beside the one above I mentioned.

@ljharb
Copy link
Member

ljharb commented Sep 9, 2020

I don't have any links, only 3-5 reports in an irc channel.

@sffc
Copy link
Author

sffc commented Sep 10, 2020

Discussion from 2020-09-10 TC39-TG2 meeting:

https://github.com/tc39/ecma402/blob/master/meetings/notes-2020-09-10.md#test262-error-in-datetime-style

The horse is out of the barn. Even if we stop throwing, there's still a version of Chrome and Firefox that have the exception. My conclusion is that we should use this as a learning experience to think more about the impact of changes like this to web compatibility and making it easier to use new features with the same code for both old and new browsers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants