Align mozIntl with Intl when working with constructors

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Blocks: 1422658
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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?
Attachment #8939963 - Flags: review+
> 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?
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94788650b26b
Align mozIntl with Intl when working with constructors. r=nalexander
Thank you Nick! I added the test for throwing when called without the |new|.

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94788650b26b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.