Expose `Q.all` compatible API from promise module

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jwalker, Assigned: jsantell)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Irakli says that Q has it now, so my 'lets not jump ahead of Q' objection is gone.
I vote we add it.
Honza - this answers the promise grouping question that you were asking about.
Assignee: nobody → rFobic
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.
> 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.
Either way I'd prefer to collaborate on each individual addition separately.
(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?
Keywords: dev-doc-needed
(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.
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.
Summary: toolkit promise should include an implementation of 'promised' → toolkit promise should include an implementation of 'promised' and 'all'
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;
  });
}
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);
  }
});
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
Summary: toolkit promise should include an implementation of 'promised' and 'all' → Expose `Q.all` compatible API from promise module
Assignee: rFobic → jsantell
Posted file GH Pull Request 842
Attachment #722462 - Flags: review?(rFobic)
Attachment #722462 - Attachment mime type: text/plain → text/html
Looks good to me. Thanks.
Comment on attachment 722462 [details]
GH Pull Request 842

Looks good, thanks!
Attachment #722462 - Flags: review?(rFobic) → review+

Comment 17

6 years ago
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

Updated

6 years ago
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.