Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spreadResolved function #307

Closed
wants to merge 1 commit into from
Closed

Add spreadResolved function #307

wants to merge 1 commit into from

Conversation

AndersDJohnson
Copy link

This commit adds a function spreadResolved similar to spread, except that it doesn't short circuit to a rejected callback if/when one of the promises is rejected. Instead, it waits until all the promises are resolved, and then calls the single provided resolved callback with variadic arguments of either fulfilled values or rejection reasons.

I find this useful when I still want to collect the values of any promises that are fulfilled, even if some of the promises might be rejected. E.g.:

Q.spreadResolved([
  promiseThatFulfills,
  promiseMaybeRejected,   // might reject, with Error object as reason
  promiseThatFulfills
], function (a, b, c) {
  if (b instanceof Error) {
    b = 'Some sensible default for b';
  }
  // go on using fulfilled values of `a` and `c`...
});

@domenic
Copy link
Collaborator

domenic commented Jun 2, 2013

Hmm I see the idea but am not too happy with parameters that can sometimes be rejection reasons and sometimes fulfillment values. Also, the correct term is actually "settled"; allResolved uses "resolved" incorrectly. (Because if you resolve a promise with another pending promise, the first promise is resolved but still pending; it is not yet settled to either fulfilled or rejected.)

Note that you can accomplish something similar today with the awkward allResolved: This is actually not true, see below

Q.allResolved([
  promiseThatFulfills,
  promiseMaybeRejected,
  promiseThatFulfills]).spread(function (a, b, c) {
  if ("exception" in b.valueOf()) {
    // .. awkward...
  }
});

With the upcoming #257, which replaces allResolved with a more accurately-named and less-awkward allSettled, you'd get access to objects of the form { state: "fulfilled", value: v } or { state: "rejected", reason: r }, which would make the code into

Q.allSettled([
  promiseThatFulfills,
  promiseMaybeRejected,
  promiseThatFulfills]).spread(function (a, b, c) {
  a = a.state === "fulfilled" ? a.value : "default A";
  b = b.state === "fulfilled" ? b.value : "default B";
  c = c.state === "fulfilled" ? c.value : "default C";
});

What do you think of that?

@AndersDJohnson
Copy link
Author

Yes, this was somewhat specific to my application, but I feel like this should be easier in general. I do prefer your proposed allSettled from #257; are there plans to release that?

I don't think your example solves the issue. See this JSFiddle. It still short-circuits to the rejected callback if any of the promises fail. Also, testing "exception" in b.valueOf() would fail when the promise resolves to a primitive type like an int, since the in operator is invalid there. I'm using hasOwnProperty('exception') instead in my latest approach.

I'm might close this pull request, as I've since found a cleaner solution that works for me for now:

function valuesOr (promises, defaultValues) {
  return promises.map(function (promise, index) {
    var value = promise.valueOf();
    return (value.hasOwnProperty('exception')) ? defaultValues[index] : value;
  });
}

Q.allResolved([
  Q.resolve(10),
  Q.reject(new Error('oops')),
  Q.resolve(20)
]).then(function (promises) {
  var results = valuesOr(promises,  ['default a', 'default b', 'default c']);
  console.log(results);
});

@kriskowal
Copy link
Owner

As @domenic, I am reluctant to mix values and errors contingent on success or failure of input promises.

We do plan to release allSettled.

@domenic
Copy link
Collaborator

domenic commented Jun 2, 2013

Oh nice gist; you're right, you can't combine spread and allResolved in the way I'd thought you would, because of the way spread does an all internally. Thankfully allSettled doesn't have the same problem :).

@kriskowal
Copy link
Owner

allSettled landed. I’m closing this since the desired behavior is simple enough to compose in terms thereof. Thank you for posting, @adjohnson916!

@kriskowal kriskowal closed this Jun 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants