Closed Bug 1433306 Opened 2 years ago Closed 27 days ago

Implement Intl.ListFormat

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox60 --- fix-optional
firefox71 --- fixed

People

(Reporter: zbraniecki, Assigned: anba)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: dev-doc-needed, parity-chrome)

Attachments

(4 files, 1 obsolete file)

Depends on: 1431957
Priority: -- → P3
Attached patch bug1433306.patch (obsolete) — Splinter Review
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?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8989795 - Flags: feedback?(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.
Attachment #8989795 - Flags: feedback?(gandalf) → feedback+
(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.
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → DUPLICATE
Duplicate of bug: 1291407
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 1291407
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?
(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #7)
> Should perhaps be {type, style} here?

Yes, ty!
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.

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

Depends on: es-intl
Blocks: es-intl
No longer depends on: es-intl

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.

"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

Depends on D40438

Depends on: 1373089
Attachment #8989795 - Attachment is obsolete: true

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.

Flags: needinfo?(andrebargull)

Pushed by csabou@mozilla.com:
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

Keywords: checkin-needed

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

Flags: needinfo?(andrebargull)

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.)

Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.