790195 - Expose `Q.all` compatible API from promise module

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Joe Walker [:jwalker]]

Irakli says that Q has it now, so my 'lets not jump ahead of Q' objection is gone.
I vote we add it.

Comment 1

[Joe Walker [:jwalker]]

See https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/promise.js#L179

Comment 2

[Joe Walker [:jwalker]]

Honza - this answers the promise grouping question that you were asking about.

Comment 3

[Joe Walker [:jwalker]]

If we're opening this up, I would propose 2 further changes:

all: Turns an array of promises into a promise for an array. If any of the promises gets rejected, the whole array is rejected immediately.

Justification: Using 'promised' for all is ugly, so everyone is going to be using a shortcut anyway. Better to provide one ourselves.

then should have the signature then(callback, errback, progress)

I think that all 3 changes can be made in a way that is 100% compatible with Q, jetpack and mozilla-central.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> all: Turns an array of promises into a promise for an array. If any of the promises > gets rejected, the whole array is rejected immediately.

From our experience one rarely needs `all` at all, since it's just possible to write functions that take promises as arguments. On the other hand bigger API surface makes learning curve steeper. So I'd be against including this into core, that's not to imply my opinions on other teams, I'd suggest implementing another module with a larger utility belt for you needs, same as we did with `promised` decorator for jetpack.

> then should have the signature then(callback, errback, progress)

You could pass `progress` listener to `then` if you want to, although it will be ignored. I'm also convinced that promises are wrong data structures for representing streaming data, I believe we discussed that and Yorik agreed on this too. Also as far as I know `progress` is not in `Q`.

I'd suggest use of linked list of promises for representing streaming data, or even better something like Microsoft's RX http://msdn.microsoft.com/en-us/data/gg577609.aspx or clojure like reducers https://github.com/Gozala/reducers or event emitters.

If you're still convinced that promises with progress listener is a best way to go about it, I would again prefer if you built it on top of promise/core without implying use of it on others.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Either way I'd prefer to collaborate on each individual addition separately.

Comment 6

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

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #5)
> Either way I'd prefer to collaborate on each individual addition separately.

I second this. Let's open one bug per feature.

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> > all: Turns an array of promises into a promise for an array. If any of the promises > gets rejected, the whole array is rejected immediately.
> 
> From our experience one rarely needs `all` at all, since it's just possible
> to write functions that take promises as arguments. On the other hand bigger
> API surface makes learning curve steeper. So I'd be against including this
> into core, that's not to imply my opinions on other teams, I'd suggest
> implementing another module with a larger utility belt for you needs, same
> as we did with `promised` decorator for jetpack.

I personally dislike `promised` and consider `all` much more usable by newcomers. However, given that they are essentially equivalent, if as I understand `promised` is part of the standard, I would go for `promised` and not `all`.

Given that `promised` is complex, though, we need kick-ass documentation!


> > then should have the signature then(callback, errback, progress)
[...]
Joe, could you open a bug on that topic?

Comment 7

[Joe Walker [:jwalker]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> > all: Turns an array of promises into a promise for an array. If any of the
> promises gets rejected, the whole array is rejected immediately.
> 
> From our experience one rarely needs `all` at all, since it's just possible
> to write functions that take promises as arguments.

You're creating APIs, I'm consuming them, which might be the difference.
We will have a utility for this if you don't.

> On the other hand bigger
> API surface makes learning curve steeper.

I don't agree, because it's one of the first questions I've been asked,
and the alternative isn't at all obvious.

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> I personally dislike `promised` and consider `all` much more usable by
> newcomers. However, given that they are essentially equivalent, if as I
> understand `promised` is part of the standard, I would go for `promised`
> and not `all`.

IIRC, both are in Q.

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> > then should have the signature then(callback, errback, progress)
> 
> You could pass `progress` listener to `then` if you want to, although it
> will be ignored. I'm also convinced that promises are wrong data structures
> for representing streaming data, I believe we discussed that and Yorik
> agreed on this too. Also as far as I know `progress` is not in `Q`.

progress is in Q, and passing a progress listener to then doesn't work.

> I'd suggest use of linked list of promises for representing streaming data,

This isn't about steaming data. This is about progress notification.

Comment 8

[Joe Walker [:jwalker]]

I'd like to re-animate this bug. To be clear on the request that we're making here:

Let's include "promised()" and "all()" in our implementation of promise.

The question isn't "should we have them?" because they're in central at least once already. I think we want to avoid different implementations of these handy methods popping up everywhere.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Every promise library I have used has had some kind of helper for promises of arrays. We need this, or else we will end up with many disparate implementations. I had to add my own for a recent patch:

/**
 * Takes an array of promises and reduces the values that they resolve to. Like
 * Array.prototype.reduce, but for arrays of promises, and returns a promise of
 * the final accumulated value.
 */
function resolveReduce(aPromises, aAccumulation, aReducer) {
  if (aPromises.length === 0) {
    return resolve(aAccumulation);
  } else {
    return resolve(aPromises[0]).then(function (val) {
      return resolve(aReducer(val, aAccumulation)).then(function (newAccumulation) {
        return resolveReduce(aPromises.slice(1), newAccumulation, aReducer);
      }, reject);
    }, reject);
  }
}

/**
 * Takes an array of promises, resolves each of them, and returns a new promise
 * that resolves to an array of each of those resolved values from the original
 * array of promises.
 */
function resolveAll(aPromises) {
  return resolveReduce(aPromises, [], function (val, acc) {
    acc.push(val);
    return acc;
  });
}

Comment 10

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

Note that Task.jsm basically trivializes waiting for any set of promises.

let promises = // Some array
let results = [];
let promised = Task.spawn(function() {
  for (let p of promises) {
    results.push(yield p);
  }
});

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Since `promised` is already exposed I'm renaming this to just exposing
`Q.all` compatible `all` function from promise module.

For the reference of Q.all API see:
http://documentup.com/kriskowal/q/#tutorial/combination

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: GH Pull Request 842

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/842

Pointer to Github pull-request

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/842#attch-to-bugzilla

Pointer to Github pull-request

Comment 15

[Joe Walker [:jwalker]]

Looks good to me. Thanks.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 722462 [details]
GH Pull Request 842

Looks good, thanks!

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/87737e7f080a8464e40a06c12534e648b1abca82
Bug 790195: Expose Promise.all sugar for promised(Array)

https://github.com/mozilla/addon-sdk/commit/ef7e1c3a7d2f758496b473b77f251afc1d7e599c
Merge pull request #842 from jsantell/Q-all-790195

Fix Bug 790195: Expose Promise.all, sugar for promised(Array), r=@gozala

Comment 18

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e468f3e947df475300719da6dd199fbe3d024551
Bug 790195: Expose Promise.all sugar for promised(Array)