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

Add support for custom compound units #19

Closed
sffc opened this issue Oct 5, 2018 · 11 comments
Closed

Add support for custom compound units #19

sffc opened this issue Oct 5, 2018 · 11 comments

Comments

@sffc
Copy link
Collaborator

sffc commented Oct 5, 2018

CLDR already provides common compound units directly as top-level units, like speed-meter-per-second. However, it also has patterns that allow for custom combinations of units.

This could be added as a field in the options enum:

{
    style: "unit",
    unit: "length-fathom",
    perUnit: "time-second"
}

That would give you fathoms per second.

ICU already supports this, and it would be trivial to add it to the spec.

@rxaviers
Copy link
Member

rxaviers commented Oct 5, 2018

From the user perspective, there's only one thing: the API either supports or not compound unit. It would be weird that some compound units are chosen via unit itself (e.g., speed-meter-per-second) and others via a combination of options unit and perUnit.

I think custom compound units is just an internal detail that should be supported, but it shouldn't be exposed as a different API to the user.

Let's be consistent and stick with either unit using -per- in its value OR this combination of options.

@sffc
Copy link
Collaborator Author

sffc commented Oct 5, 2018

Interesting point.

Let's consider meter per second squared. This is currently one of the first-class CLDR units.

If we do the string approach, which would we want of the following?

  1. meter-per-second-squared
  2. meter-per-second-per-second

If we do the API approach, how would you put multiple units in the numerator or denominator, such as in meter per second squared? Maybe something like this?

new Intl.NumberFormat("es-mx", {
    style: "unit",
    unit: {
        numerator: ["meter"],
        denominator: ["second", "second"]
    }
});

In the string approach, would we want to support putting multiple units on top, in the numerator, like we can do with the API approach?

In either approach, how do we handle the case when locale data isn't capable of rendering the unit? For example, locale data is currently only smart enough to do one numerator and one denominator, except in the special case of units like meter-per-second-per-second, which have their own entry. We can't just check against a list of approved units any more; a proper check would be much more nuanced, possibly beyond the capability of the spec. We would maybe need to revisit #11.

@rxaviers
Copy link
Member

rxaviers commented Oct 5, 2018

With respect to your question about multiple compound unit level (the nested-per), I would consider this a separate problem. Anyway, if this is supported, I would expect format(1, "meter-per-second-squared") to return "1 m/s²" (what we have so far) and format(1, "meter-per-second-per-second") to return "1 m/s/s". I would not expect the API to figure out that per-second-per-second should be per-second-squared. Anyway, I am not sure how useful this would be nor how we would use CLDR to accomplish that.

@sffc
Copy link
Collaborator Author

sffc commented Oct 5, 2018

We already have to be a little bit smart to detect when a unit is "special" and has its own CLDR pattern. I don't think coalescing multiple units together would be a significant jump.

That being said, I'm kind-of leaning toward keeping a strict whitelist of units supported. CLDR already covers most common compound units, and maybe we can add to the EcmaScript spec a handful of compound units important to web development, like megabytes per second. Then it's clear to developers and to users what's available and what's not.

@littledan
Copy link
Member

I'm a little worried about adding a new string-based syntax of our invention, even if it's relatively simple. We could avoid that by either always using this separated unit/perUnit form, or always using something from the whitelist.

@sffc sffc mentioned this issue Oct 24, 2018
@sffc
Copy link
Collaborator Author

sffc commented Oct 24, 2018

The decision from the ECMA 402 meeting was to expose the specific list of units anointed by CLDR, which may grow over time, and which includes certain popular compound units like meters per second. The comments included:

  • SC: The compound units that CLDR specifies has specific data, and CLDR has curated them. That's the difference between the special compound unit and the custom ones. The curated ones are the ones most likely in high demand, since CLDR curates data on request.
  • MM, regarding the possibility of adding a custom list of units specific to ECMA 402: The concern I have is that CLDR could change things in some way we didn't forsee. It would be shortsighted to do something here given the longevity of JavaScript. If the goal is to have an implementation before CLDR can get its act together, I think it'd be better to approach CLDR first.
  • DE: I think we should start out being more conservative.
  • SC: If we wanted to allow arbitrary units, it opens up the can of worms about how we want to design the API. I think we can add compound units in the future.
  • SC: Maybe we can work with CLDR to work on a spec for custom unit identifiers. I think the value of having every unit being a string is very nice.
  • SC: We're talking about the ECMAScript spec--even if CLDR supports something, we don't have to support it in ES. I'm not very happy with the current CLDR "per" unit specification, with its limitations. I'd rather see, if we allow arbitrary combinations, to have any number of numerators and any number of denominators. Since we're designing a new specification, I'd rather not carry forward this limitation.

rxaviers expressed opposition:

  • RX: Although CLDR doesn't include the compound unit explicitly, the algorithm is pretty clear about how it should generate that from existing data. Although there's no explicit compound units there, the units are there and the algorithm is clear. I think it's not there not because it's not useful, but because it doesn't need to be there.

Closing this ticket because there is no need to modify the proposal.

@sffc sffc closed this as completed Oct 24, 2018
@rxaviers
Copy link
Member

Thanks for the minutes @sffc

@sffc
Copy link
Collaborator Author

sffc commented Oct 30, 2018

So on #17, if I understand Rafael's suggestion correctly, the spec actually will include a way to define arbitrary compound units as a string.

This would invalidate most of my trepidations about introducing this feature into ECMAScript. I was concerned around the elegance of having a string unit identifier and the complexity of having to design an API for it.

I'm reopening this ticket to continue discussion.

@sffc sffc reopened this Oct 30, 2018
@sffc
Copy link
Collaborator Author

sffc commented Oct 30, 2018

I posted a couple comments over on the CLDR thread.

One question we need to answer is how to handle the case when the implementation does not have the data to format a unit correctly. With the latest LDML revision, if accepted, the rule could be like this, where the "list below" refers to the full list of units that I pasted from CLDR into the ES spec:

  1. If the unit is in the list below, return true.
  2. If the unit is a compound unit, and both the numerator and the denominator are in the list below, return true.
  3. Return false.

@sffc
Copy link
Collaborator Author

sffc commented Nov 23, 2018

I updated the spec with the new "core unit identifiers" syntax last week in 7606464.

WRT correctness logic: we're still throwing a RangeError, but only if the unit nor the decomposed simple units exist in CLDR data. This way, we are getting full CLDR data coverage and only rejecting things for which we legitimately don't know how to format.

@sffc sffc closed this as completed Nov 23, 2018
@littledan
Copy link
Member

Great work, @rxaviers and @sffc!

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

3 participants