895185 - Implement Promise.all()

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Implement Promise.every()

Promise.jsm is great and all but we're going to have trouble converting existing code to the JSM without some of the basic helpers that deal with a collection of promises.

When converting about:newtab to Promise.jsm I was blocked by not having Promise.every() so that's what I started with. The implementation of Promise.every() in this patch adheres to the DOM spec:

http://dom.spec.whatwg.org/#dom-promise-every

Comment 1

[:Paolo Amadini]

Comment on attachment 777458 [details] [diff] [review]
Implement Promise.every()

I think the "every" function can be a useful helper, and the fact that it is
part of the specification makes it a candidate for implementation directly in
the "Promise.jsm" module.

However, we don't want a "synchronous" callback implementation here. This
doesn't give any additional value to the Chrome code that's using this module.

So, we should implement the function in a very simple way, on top of the other
existing public functions (read "defer"), without using any of the internals.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Ok, sounds good to me. I agree with you about the sync flag, I was just sticking to the spec :)

Comment 3

[:Paolo Amadini]

So, is this a substitute for Promise.all from the SDK module?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Implement Promise.every(), v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Paolo Amadini [:paolo] from comment #3)
> So, is this a substitute for Promise.all from the SDK module?

Yes, this should do the same.

Comment 6

[:Paolo Amadini]

Comment on attachment 777719 [details] [diff] [review]
Implement Promise.every(), v2

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

::: toolkit/modules/Promise.jsm
@@ +177,5 @@
> +   * Returns a promise that is resolved or rejected when all values are
> +   * resolved or any is rejected.
> +   *
> +   * @param aValues
> +   *        List of promises that may be pending, resolved, or rejected. When

List of -> Array of

Do we support array-like objects as well?

If any of the elements in the array is not a promise, what should we do? Should we forward it as a resolution value, throw an exception, or reject the returned promise? (We'll need tests for that)

@@ +182,5 @@
> +   *        all are resolved or any is rejected, the returned promise will be
> +   *        resolved or rejected as well.
> +   *
> +   * @return A new promise that is fulfilled when all values are resolved or
> +   *         that is reject when any of the values are rejected.

Please specify which resolution value and rejection reason the returned promise assume.

@@ +188,5 @@
> +  every: function (aValues)
> +  {
> +    if (!aValues.length) {
> +      return Promise.resolve(undefined);
> +    }

I wonder whether we should explicitly throw a descriptive exception if the provided argument is not array-like?

@@ +193,5 @@
> +
> +    let index = 0;
> +    let countdown = aValues.length;
> +    let deferred = Promise.defer();
> +    let args = new Array(countdown);

nit: args -> resolutionValues

@@ +196,5 @@
> +    let deferred = Promise.defer();
> +    let args = new Array(countdown);
> +
> +    for (let value of aValues) {
> +      let currentIndex = index++;

A classic "for" loop seems simpler here. Are there reasons for using enumeration?

@@ +199,5 @@
> +    for (let value of aValues) {
> +      let currentIndex = index++;
> +
> +      value.then(function (aArgument) {
> +        args[currentIndex] = aArgument;

nit: aArgument -> aValue

Comment 7

[:Paolo Amadini]

David, can you take a look at comment 6 and tell us what you think makes most sense?

Dave, if you can also look at this, we'll need super-review on the API.

Comment 8

[Dave Townsend [:mossop]]

sr+

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Paolo Amadini [:paolo] from comment #6)
> If any of the elements in the array is not a promise, what should we do?
> Should we forward it as a resolution value, throw an exception, or reject
> the returned promise? (We'll need tests for that)

I would forward it as a resolution value.
This is consistent with the natural implementation of |every| in Task.jsm:

otherImplementationOfEvery = function(promises) {
  return Task.spawn(function() {
    let values = [];
    for (let x of promises) {
      values.push((yield x));
    }
    throw new Task.Result(values);
  });
}

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Implement Promise.every(), v3

(In reply to Paolo Amadini [:paolo] from comment #6)
> If any of the elements in the array is not a promise, what should we do?
> Should we forward it as a resolution value, throw an exception, or reject
> the returned promise? (We'll need tests for that)

I took Yoric's suggestions and forwarded the value. Added tests.

> I wonder whether we should explicitly throw a descriptive exception if the
> provided argument is not array-like?

Added.

> > +    for (let value of aValues) {
> > +      let currentIndex = index++;
> 
> A classic "for" loop seems simpler here. Are there reasons for using
> enumeration?

It's not really simpler. It's just different :) Changed it to a classic for loop anyway.

Comment 11

[:Paolo Amadini]

Comment on attachment 778445 [details] [diff] [review]
Implement Promise.every(), v3

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

Looks OK, with the comment change below.

::: toolkit/modules/Promise.jsm
@@ +184,5 @@
> +   *
> +   * @return A new promise that is fulfilled when all values are resolved or
> +   *         that is rejected when any of the values are rejected. Its
> +   *         resolution value will be an array of all resolved values in the
> +   *         given order. The reject reason will be forwarded from the first

"Its resolution value will be an array of all resolved values in the given order, or undefined if aValues is an empty array."

(for what it's worth, I think this inconsistent behavior with empty arrays is a bug in the specification, but we have to live with it.)

Do you think we may have issues when converting code from "Promise.all" to "Promise.every" because of this behavior?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Paolo Amadini [:paolo] from comment #11)
> Do you think we may have issues when converting code from "Promise.all" to
> "Promise.every" because of this behavior?

I can't imagine that we have too many cases where we actually pass an empty list to Promise.all/every and expect it to have a meaningful outcome. I think we should rather stick to the spec, that should give us less problems in the future. Converting a few occurrences of the old code, if any, should not be too much work. I think it would be helpful to add this as a note to bugs that switch to the new Promise.jsm.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/bc59f7e483c5

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/bc59f7e483c5

Comment 15

[:Paolo Amadini]

We got this wrong, see the example here:

http://dom.spec.whatwg.org/#i-promise-you-an-introduction

The spec is not crystal clear, when it mentions "values" and "arguments" it means
the expanded function parameters, not arrays.

We can't implement that at present, because we never invoke resolution callbacks
with multiple arguments (and I'm not sure we should in the future, the Promise.jsm
API is already quite different from DOM Promises).

To solve this, we may just rename "every" back to "all" (to clarify that we are
using a different interface) and fix the issue of resolving to "undefined" by
resolving to an empty array if the given array is empty in the first place.

(It made sense to say "undefined" in the DOM Promises specification because it
actually meant an empty argument list.)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Rename Promise.every() to .all() to reduce confusion about our differing implementation

Comment 17

[:Paolo Amadini]

Comment on attachment 783738 [details] [diff] [review]
Rename Promise.every() to .all() to reduce confusion about our differing implementation

Thanks!

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/036e937a22a2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/036e937a22a2