1428172 - Align mozIntl with Intl when working with constructors

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Currently the mozIntl API is misaligned with Intl API when it comes to constructors:

```
let dtf = new Intl.DateTimeFormat(locales, options);

let dtf2 = mozIntl.createDateTimeFormat(locales, options);
```

Before we extend the mozIntl to cover other Intl constructors (bug 1422658), I'd like to align it.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1428172 - Align mozIntl with Intl when working with constructors.

Review commit: https://reviewboard.mozilla.org/r/210234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210234/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8939963 [details]
Bug 1428172 - Align mozIntl with Intl when working with constructors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210234/diff/1-2/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8939963 [details]
Bug 1428172 - Align mozIntl with Intl when working with constructors.

https://reviewboard.mozilla.org/r/210234/#review215980

I am perhaps not expert enough with JS classes and constructors to review this, but if you're confident it's correct, it lgtm.

::: toolkit/components/mozintl/mozIntl.js:80
(Diff revision 2)
>  
> -    let resolvedLocales =
> +    let DateTimeFormat = this._cache.DateTimeFormat;
> -      this._cache.DateTimeFormat.supportedLocalesOf(getLocales(locales));
>  
> +    class MozDateTimeFormat extends this._cache.DateTimeFormat {
> +      constructor(locales, options, ...args) {

I vaguely recall there's some thing about allowing a constructor function to be called as a function.  Can you add a test that `foo = Intl.DateTimeFormat(...)` and `foo2 = mozIntl.DateTimeFormat(...)` do the same thing?

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

> I vaguely recall there's some thing about allowing a constructor function to be called as a function.  Can you add a test that `foo = Intl.DateTimeFormat(...)` and `foo2 = mozIntl.DateTimeFormat(...)` do the same thing?

There's a consensus on old Intl constructors that allows them to work as function calls. See:
 - https://github.com/tc39/ecma402/pull/84
 - https://bugzilla.mozilla.org/show_bug.cgi?id=1328386

This is only added for backward compatibility reasons and all new formatters (like Intl.PluralRules) throw.

It would be fairly complex to replicate this consensus for mozIntl.DateTimeFormat and I'm not sure if it's really worth it. I'm totally ok with throwing if called without `new`.

Do you think there's a reason to push for it or would you be ok if I added a test that verifies that `mozIntl.DateTimeFormat()` throws like all modern Intl.* constructors?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> > I vaguely recall there's some thing about allowing a constructor function to be called as a function.  Can you add a test that `foo = Intl.DateTimeFormat(...)` and `foo2 = mozIntl.DateTimeFormat(...)` do the same thing?
> 
> There's a consensus on old Intl constructors that allows them to work as
> function calls. See:
>  - https://github.com/tc39/ecma402/pull/84
>  - https://bugzilla.mozilla.org/show_bug.cgi?id=1328386
> 
> This is only added for backward compatibility reasons and all new formatters
> (like Intl.PluralRules) throw.
> 
> It would be fairly complex to replicate this consensus for
> mozIntl.DateTimeFormat and I'm not sure if it's really worth it. I'm totally
> ok with throwing if called without `new`.
> 
> Do you think there's a reason to push for it or would you be ok if I added a
> test that verifies that `mozIntl.DateTimeFormat()` throws like all modern
> Intl.* constructors?

No.  The important thing is that the tests assert the desired state, which looks like it can be "throw without new".  So add that assertion and explain why there's an observable different between `Intl` and `mozIntl`, i.e., why you're doing what you're doing.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8939963 [details]
Bug 1428172 - Align mozIntl with Intl when working with constructors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210234/diff/2-3/

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8939963 [details]
Bug 1428172 - Align mozIntl with Intl when working with constructors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210234/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94788650b26b
Align mozIntl with Intl when working with constructors. r=nalexander

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you Nick! I added the test for throwing when called without the |new|.

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/94788650b26b