1433306 - Implement Intl.ListFormat

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P3)

Description

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

Intl.ListFormat is now Stage 3 - https://github.com/tc39/proposal-intl-list-format/

Comment 1

[André Bargull [:anba]]

Attachment: bug1433306.patch

First draft for a patch to add Intl.ListFormat, therefore f? only for now.

The (public) API for ICU doesn't seem to provide enough functionality to support all features of the Intl.ListFormat proposal. For example Intl.ListFormat.prototype.formatToParts is currently not implementable <https://ssl.icu-project.org/trac/ticket/13754>, the "disjunction" and "unit" types can only be implemented by using an ICU internal C++ API, and the actually supported locales cannot be determined. All this makes me wonder if we should really pursue implementing the proposal at this point or if we rather should wait until ICU provides the necessary API to cleanly implement Intl.ListFormat?

Comment 2

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

Comment on attachment 8989795 [details] [diff] [review]
bug1433306.patch

Review of attachment 8989795 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Anba! This looks good!

I agree that we may want to wait until ICU improves, but having a first draft helps understanding how the spec holds.

::: js/src/builtin/intl/ListFormat.js
@@ +166,5 @@
> +
> +    var stringList = [];
> +    for (var element of allowContentIter(list)) {
> +        // FIXME: spec issue - the proposal currently requires a strict string type check
> +        // instead of calling ToString. That's probably a spec bug?!

I don't think it's a bug. We discussed it several time and I suggested using `ToLocaleString` on the list elements to allow for nice nested resolution of dates, numbers etc., but others were concerned about trying to pass option bags from list format to list element formatters.

We settled on strictly requiring that every element is a string for now.

Comment 3

[André Bargull [:anba]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> I don't think it's a bug.

Yeah, I found the issue describing the motivation for requiring string values after uploading the patch. It's still a bit confusing, therefore I'm going to file an issue requesting to add a note before the string type check [1] in the proposal, so future spec readers see that the current behaviour is indeed not a spec bug. And on our side we want to have a good error message, so users know what to do when passing non-string values to the format() method. :-)

[1] Similar to the note in CanonicalizeLocaleList where a TypeError is thrown when non-string and non-object values are passed to the operation.

Comment 5

[André Bargull [:anba]]

This bug has a more recent patch than bug 1291407, so if we want to close one of the two bugs, it should be the other one.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8989795 [details] [diff] [review]
bug1433306.patch

Review of attachment 8989795 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/non262/Intl/ListFormat/constructor.js
@@ +45,5 @@
> +}
> +
> +for (let type of ["conjunction", "disjunction", "unit"]) {
> +    for (let style of [null, false, "", "standard", "default", "long\0"]) {
> +        assertThrowsInstanceOf(() => new Intl.ListFormat("en", {style}), RangeError);

Should perhaps be {type, style} here?

Comment 8

[André Bargull [:anba]]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #7)
> Should perhaps be {type, style} here?

Yes, ty!

Comment 9

[Jeff Walden [:Waldo]]

The v8 peoples are implementing a hackaround in advance of an ICU API:

https://cs.chromium.org/chromium/src/v8/src/objects/js-relative-time-format.cc?q=js-relative&sq=package:chromium&g=0&l=411

Distasteful, but changes in to-parting are something users already have to work with anyway, so it doesn't seem wholly un-shippable to me.

Comment 10

[709922234]

https://developers.google.com/web/updates/2018/12/intl-listformat

Comment 11

[Alexandre Folle de Menezes]

This has shipped on Chrome 72, so maybe a "chrome-parity" tag is needed.

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1433306 - Part 1: Handle RelativeTimeFormat in getInternals and some clean-ups. r=jwalden!

RelativeTimeFormat.cpp doesn't call getInternals() with an unresolved
internals object, so this didn't blow up in the past.

Drive-by fixes:

• GlobalObject::createConstructor is a static function, so it doesn't need to
  be called with an instance.
• Remove the unused "methodName" argument from getRelativeTimeFormatInternals.

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1433306 - Part 2: Implement Intl.ListFormat stage 3 proposal. r=jwalden!

"disjunction" and "unit" types aren't yet supported, because ICU doesn't
provide a C-API for this functionality. "short" and "narrow" styles aren't
supported for the same reason.

Depends on D40436

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1433306 - Part 3: Enable test262 tests for Intl.ListFormat. r=jwalden!

Depends on D40437

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1433306 - Part 4: Reimport test262 tests. r=jwalden!

Depends on D40438

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:anba, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[André Bargull [:anba]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c7f5516042c6065a38ff29ec6185aabf22b923
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc65eff0bf0353ccad40986ef3694c160ccc873

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b818a65ca8da
Part 1: Handle RelativeTimeFormat in getInternals and some clean-ups. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/d701e5149249
Part 2: Implement Intl.ListFormat stage 3 proposal. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/78466d478f09
Part 3: Enable test262 tests for Intl.ListFormat. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/383e2052153e
Part 4: Reimport test262 tests. r=jwalden

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hey anba, are you dealing with https://github.com/tc39/proposal-intl-list-format/issues/45 already?

Comment 20

[André Bargull [:anba]]

The patch doesn't actually enable Intl.ListFormat by default, but instead it needs to be manually added to the Intl object through the shell-only helper function addIntlExtras. Shipping Intl.ListFormat is right now blocked by bug 1589095 and probably also the open proposal resp. CLDR issue you've linked to. (I've only landed the patches because they've already been reviewed and to get them out of my queue.)

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b818a65ca8da
https://hg.mozilla.org/mozilla-central/rev/d701e5149249
https://hg.mozilla.org/mozilla-central/rev/78466d478f09
https://hg.mozilla.org/mozilla-central/rev/383e2052153e

Comment 22

[Mike Conca [:mconca]]

(In reply to André Bargull [:anba] from comment #20)

The patch doesn't actually enable Intl.ListFormat by default, but instead it needs to be manually added to the Intl object through the shell-only helper function addIntlExtras. Shipping Intl.ListFormat is right now blocked by bug 1589095 and probably also the open proposal resp. CLDR issue you've linked to. (I've only landed the patches because they've already been reviewed and to get them out of my queue.)

Bug 1589095 landed a couple of weeks ago. Is this still blocked on the CLDR issue?

Comment 23

[André Bargull [:anba]]

No, it's no longer blocked by the CLDR issue. A workaround was added to ICU to properly support Spanish (and Hebrew). As part of bug 1589095, Intl.ListFormat was enabled by default and will be available starting Firefox 78.

Comment 24

[Mike Conca [:mconca]]

(In reply to André Bargull [:anba] from comment #23)

No, it's no longer blocked by the CLDR issue. A workaround was added to ICU to properly support Spanish (and Hebrew). As part of bug 1589095, Intl.ListFormat was enabled by default and will be available starting Firefox 78.

Thank you

Comment 25

[Florian Scholz (Open Web Docs)]

For docs, see https://bugzilla.mozilla.org/show_bug.cgi?id=1589095#c7