Implement Promise.all()

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 777458 [details] [diff] [review]
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
Attachment #777458 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

5 years ago
Blocks: 895359

Comment 1

5 years ago
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.
Attachment #777458 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 2

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

Updated

5 years ago
Blocks: 856878

Comment 3

5 years ago
So, is this a substitute for Promise.all from the SDK module?
(Assignee)

Comment 4

5 years ago
Created attachment 777719 [details] [diff] [review]
Implement Promise.every(), v2
Attachment #777458 - Attachment is obsolete: true
Attachment #777719 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 5

5 years ago
(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

5 years ago
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
Attachment #777719 - Flags: review?(paolo.mozmail)

Comment 7

5 years ago
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.
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(dteller)
sr+
Flags: needinfo?(dtownsend+bugmail)
(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);
  });
}
Flags: needinfo?(dteller)
(Assignee)

Comment 10

5 years ago
Created attachment 778445 [details] [diff] [review]
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.
Attachment #777719 - Attachment is obsolete: true
Attachment #778445 - Flags: review?(paolo.mozmail)

Comment 11

5 years ago
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?
Attachment #778445 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bc59f7e483c5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 years ago
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.)
Flags: needinfo?(ttaubert)
Flags: needinfo?(dteller)
(Assignee)

Comment 16

5 years ago
Created attachment 783738 [details] [diff] [review]
Rename Promise.every() to .all() to reduce confusion about our differing implementation
Attachment #783738 - Flags: review?(paolo.mozmail)
Flags: needinfo?(ttaubert)

Comment 17

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

Thanks!
Attachment #783738 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Updated

5 years ago
Flags: needinfo?(dteller)
Summary: Implement Promise.every() → Implement Promise.all()

Updated

5 years ago
Whiteboard: [Async]

Updated

5 years ago
Component: General → General
Product: Add-on SDK → Toolkit
Target Milestone: --- → mozilla25
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.