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

Consider error checking for using dateStyle/timeStyle together with other options #2

Closed
littledan opened this issue Sep 8, 2018 · 19 comments

Comments

@littledan
Copy link
Member

The current spec text does not read the other options if dateStyle/timeStyle are set. Another option would be to throw some sort of error in this case. This might be better for debugging. cc @rxaviers

@rxaviers
Copy link
Member

rxaviers commented Sep 10, 2018

👍 When orthogonal options are used together, it's a clear API misuse. It helps development to get an error early than having the API to silently pick one option over the other.

PS: Out of the scope here, but a similar scenario happens on https://github.com/sffc/proposal-unified-intl-numberformat, e.g., using style: "unit", but currency: "USD" (option that is specific for currency style).

@sffc
Copy link

sffc commented Sep 12, 2018

My understanding from the ECMA 402 meeting a few months ago was that it was desirable behavior to silently ignore options that are unused in the options bag. It's not always API misuse. See Eemeli's comment here:

https://github.com/tc39/ecma402/blob/master/meetings/notes-2018-03-16.md

Eemeli: As a library developer, it seems easier and cleaner to have the style argument. The semantics are more like, when I pass a currency, that means, "if a currency is needed, that is X", etc. Otherwise, I’d need to maintain different objects depend on what thing is being formatted, so it’s messier.

@littledan
Copy link
Member Author

littledan commented Sep 24, 2018

We discussed this issue in the September 2018 TC39 ECMA-402 meeting and decided to not use stricter error checking, matching the previous discussion.

@mathiasbynens
Copy link
Member

We discussed this issue in the September 2018 TC39 meeting

Did you post this from the future? ;)

How about creating separate but symmetrical API methods for each style? That would avoid the problem altogether.

@littledan
Copy link
Member Author

@mathiasbynens We already decided not to do that for currency. I don't really understand the purpose of the multi-method approach; could you elaborate?

@mathiasbynens
Copy link
Member

You wouldn’t need the style property anymore, and each method could then accept an object with a consistent shape. It also completely avoids the problem of what to do when another property is present (i.e. this issue) since it would then just work similarly to all existing methods that accept option bag arguments.

What’s the benefit of stuffing both features into the same method? It encourages polymorphism for seemingly no good reason.

@rxaviers
Copy link
Member

rxaviers commented Oct 2, 2018

@mathiasbynens, a dedicated method Intl.UnitFormat was the initial proposal, but like @littledan mentioned, NumberFormat already includes (the orthogonal) currency (since the old 1st version of Ecma-402 API) and then the unified API proposal gained traction.

In the unified API proposal, Intl.NumberFormat is extended to support units (orthogonal to number and currency) and also extra options: some are common to all (e.g., compact notation, sign display), some are specific to particular features (e.g., currency accounting format for currency only).

Given this context, are you in line with the path this is going? Any ideas?

@sffc
Copy link

sffc commented Oct 2, 2018

There are a lot of ways that we could design multiple different styles, each with a different set of options. Here are just a few:

(123).toLocaleString("en-us", {
    style: "unit",  // explicitly says that we are in unit mode
    unit: "length-meter",
    unitWidth: "short"
    // if currency field is present, ignore it
});

(123).toLocaleString("en-us", {
    unit: "length-meter",  // presence of key implies unit mode
    unitWidth: "short"
    // if currency field is present, throw an exception
});

(123).toLocaleString("en-us", {
    style: {
        type: "unit",
        unit: "length-meter",
        unitWidth: "short"
    }
});

(123).toLocaleString("en-us", {
    unit: {
        id: "length-meter",
        width: "short"
    }
    // presence of currency field would throw exception
});

All of these options have advantages and disadvantages. We decided on the first one when discussing the units proposal, which is a precedent that we should follow in similar APIs. We can reopen that discussion while the NumberFormat proposal is still in Stage 2, but I personally don't see a super compelling case for any of the other options over the first.

@sffc
Copy link

sffc commented Oct 2, 2018

With regard to the comment about separate methods:

Let's say we split currency, unit, percent, and standard number formatting all into different API functions. Fine; we have 4 methods, all of which accept the same options for rounding style, notation, etc. However, next year we realize that fraction rounding needs a different set of options than significant digits rounding. Since we want to split everything up into separate methods, now we have 8 methods, one for each combination of style and rounding. Things get ugly very fast.

In Java, the style set forth by libraries like Google Guava is the "fluent" pattern. It looks like this:

NumberFormatter.withLocale(ULocale.US)
    .unit(MeasureUnit.METER)
    .precision(Precision.integer())
    .notation(Notation.scientific())

That's fine, and it enforces at compile time a namespace to make sure you don't have conflicting settings or settings that have no effect at runtime.

However, we're in JavaScript, not Java. Java has no notion of object literals, which are a foundational building block of JavaScript APIs. It would be artificial and unauthentic to take the Java notion of methods and map it onto JavaScript.

@rxaviers
Copy link
Member

rxaviers commented Oct 2, 2018

It's not my goal to extend (and rehash) this much... Anyway, one quick question:

Fine; we have 4 methods, all of which accept the same options for rounding style, notation, etc. However, next year we realize that fraction rounding needs a different set of options than significant digits rounding. Since we want to split everything up into separate methods, now we have 8 methods, one for each combination of style and rounding. Things get ugly very fast.

The 4 methods would accept the new options, no? Sorry, I didn't follow the 8 methods...

@sffc
Copy link

sffc commented Oct 2, 2018

It's not my goal to extend (and rehash) this much... Anyway, one quick question:

Fine; we have 4 methods, all of which accept the same options for rounding style, notation, etc. However, next year we realize that fraction rounding needs a different set of options than significant digits rounding. Since we want to split everything up into separate methods, now we have 8 methods, one for each combination of style and rounding. Things get ugly very fast.

The 4 methods would accept the new options, no? Sorry, I didn't follow the 8 methods...

The reason that was suggested for separating styles into separate methods was to avoid the problem of passing an option to the object literal that is used in some situations but not in others. (By the way, I don't like the word "style"; "unit type" would be more precise.)

What I'm trying to say is that this issue is not unique to the four styles. Rounding strategies and notation are two examples of things besides styles that have different settings depending on which type you choose. For example, maximumFractionDigits is implicitly ignored when maximumSignificantDigits is set. Soon, the setting compactDisplay will be ignored if a non-compact notation is chosen.

If you wanted to be truly "type-safe" within the framework where all settings are specified in a single object literal, you need something like this, where each constructor accepts a very specific set of options in the object literal:

  • new Intl.CurrencyFractionFormat()
  • new Intl.CurrencyFractionScientificFormat()
  • new Intl.CurrencyFractionCompactFormat()
  • new Intl.CurrencySignificantFormat()
  • new Intl.CurrencySignificantScientificFormat()
  • new Intl.CurrencySignificantCompactFormat()
  • ...

This is where I got the explosion from 4 to 8. Adding in the three notation types explodes that again to 24.

@rxaviers
Copy link
Member

rxaviers commented Oct 5, 2018

Thanks for the clarification, makes sense

@littledan
Copy link
Member Author

Should we discuss this again in an Intl meeting?

@sffc
Copy link

sffc commented Oct 8, 2018

To recap, there are at least two reasons specifically in favor of ignoring unused options:

  1. Eemeli's comment about having a pre-populated option bag.
  2. The precedent in Intl.NumberFormat, which already has a style attribute.

My opinion is still that we should keep with existing behavior and silently ignore unused options. I don't think it should be part of the spec, but maybe we could add a feature to eslint that catches when unused properties are hard-coded into an options object.

I think the people with an opinion on this issue have already commented, and we've already discussed it at two ECMA 402 meetings. I don't see a reason to add it to the agenda unless there are new revelations.

@littledan
Copy link
Member Author

I like the eslint idea. Is anyone interested in trying out an implementation? (Would this be possible with TS types?)

@nathanhammond
Copy link
Member

Making concrete, here is the author failure mode:

new Intl.DateTimeFormat('en', {
  dateStyle: 'short',

  // The following values are disregarded:
  year: 'numeric',
  month: 'long',
  day: 'numeric'
});
  • Making this an error would be a change in behavior if for some reason you presently had an ignored dateStyle or timeStyle field in the object. (This change is likely web compatible, but worth noting.)
  • "No error" is consistent with the behavior of other formatters.
  • It's likely lintable in most scenarios for eslint (though the places where it would fail would be dynamic things which would be more likely to contain bugs), and may be completely lintable for TypeScript.

I don't find providing a hard Error to prevent author-supplied-configuration-data conflicts ergonomic given that it may only conditionally show up in published code and agree with the conclusion here.

@sffc
Copy link

sffc commented Nov 27, 2019

I know this issue was resolved over a year ago, but I've had some changing feelings on it.

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
Copy link
Member

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

sffc commented Dec 12, 2019

My fault for posting in a closed issue. I copied your comments to #40. Let's discuss over there.

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

No branches or pull requests

6 participants