(@fluent/bundle) Forbid setting NUMBER's currency

[stasm]

If one of the named arguments passed into NUMBER is currency, we should remove it from the options bag. Right now this PR just ignores it silently, but perhaps it would be better to explicitly throw in NUMBER instead. @Pike, wdyt?

I also wonder if we should disallow NUMBER's style, as well. Are there scenarios when we'd want the localization to choose a different style (e.g. percent) than the base (e.g. currency)? I guess one such example is when the base is not explicitly defined in the source, in which case decimal is used. This can happen in the declarative bindings where there's no way to pass an instance of FluentNumber (like in DOMLocalization).

[stasm]

I added a second commit which changes the silent error behavior to an explicit one: the NUMBER built-in throws when one of the options is named currency. While I think it's a good idea to be more strict about this now, I dislike how it makes NUMBER(1234, currency: "EUR") produce NUMBER() (because the entire builtin throws) while NUMBER(1234, style: "currency") produces an un-formatted 1234 because the TypeError: Currency code is required with currency style. happens later in FluentNumber.toString().

[Pike]

Oh rabbit hole, where are you when I need you? Oh, nice, there you are.

I'd go as far as to whitelist valid arguments. But that's hard for backwards compat. The story is full of sadness, like plain calls to NUMBER($FOO) in various places, 🤷‍♂ .

Note, there's already pre-existing documentation on restricting the options in https://projectfluent.org/fluent/guide/functions.html.

I'd blacklist unit and notation, additionally, and whitelist unitDisplay.

[stasm]

Nice documentation find. It looks it was written a long time ago, in a rather wishful tone :)

If I'm understanding correctly, you're suggest to do the following?

• Throw when a forbidden option is passed to NUMBER():

    localeMatcher
    style
    currency
    unit
    notation
  

• Ignore all options other than the known ones:

    numberingSystem (not sure about this one)
    currencyDisplay
    unitDisplay
    useGrouping
    minimumIntegerDigits
    minimumFractionDigits
    maximumFractionDigits
    minimumSignificantDigits
    maximumSignificantDigits
  

• Don't require the known options so that NUMBER($foo) continues to work.

I like this, but I'm afraid of breaking a combination of data-l10n-args and style somewhere where I'm not aware of them being used:

foo = Progress: {NUMBER($prog, style="percent")}
<span data-l10n-id="foo" data-l10n-args='{"prog": 0.75}' />

FWIW, I haven't found anything like that in gecko-strings not in l10n-central.

[stasm]

I changed the approach again to explicitly listing all allowed options, and silently ignoring all other options. This approach gives us good fallback for both unknown and forbidden options, and lets us strictly control allowed options in the future, too.

[Pike]

Thanks, the design looks right.

I'd be more restrictive on params that can lead to different meanings for the same number.

The tests are probably OK, but they're also all over the place. When we think about tests and their usefulness for other resolvers, we should make sure that details of the NUMBER and DATETIME implemetations are restricted to as few test files/cases as possible.

[stasm]

Thanks, @Pike. I agree about being more restrictive in terms of which options are allowed; I pushed the changes you requested. Good points about the tests, too.