petkaantonov / bluebird

:bird: :zap: Bluebird is a full featured promise library with unmatched performance.
http://bluebirdjs.com
MIT License
20.45k stars 2.34k forks source link

promise.any() throws error on reject #677

Closed vitaly-t closed 9 years ago

vitaly-t commented 9 years ago

TEST

promise.any([
    promise.reject('stop')
]).
    then(function () {
    }, function (reason) {
        console.log(reason);
    });

OUTPUT

Warning: a promise was rejected with a non-error: [object String]
    at warn (D:\NodeJS\tests\node_modules\bluebird\js\main\debuggability.js:256:
19)
    at Promise._warn (D:\NodeJS\tests\node_modules\bluebird\js\main\debuggabilit
y.js:77:12)
    at Promise._rejectCallback (D:\NodeJS\tests\node_modules\bluebird\js\main\pr
omise.js:411:14)
    at Function.Promise.reject.Promise.rejected (D:\NodeJS\tests\node_modules\bl
uebird\js\main\promise.js:184:9)
From previous event:
    at Promise.longStackTracesCaptureStackTrace [as _captureStackTrace] (D:\Node
JS\tests\node_modules\bluebird\js\main\debuggability.js:216:19)
    at Function.Promise.reject.Promise.rejected (D:\NodeJS\tests\node_modules\bl
uebird\js\main\promise.js:183:9)
    at Object.<anonymous> (D:\NodeJS\tests\test.js:140:13)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3
{ [AggregateError: aggregate error] '0': 'stop', length: 1 }

Tested with versions 2.9.30 and 3.0.0

benjamingr commented 9 years ago

What did you expect it to do :D?

On Sun, Jun 28, 2015 at 2:25 PM, Vitaly Tomilov notifications@github.com wrote:

TEST

promise.any([ promise.reject('stop') ]). then(function () { }, function (reason) { console.log(reason); });

OUTPUT

Warning: a promise was rejected with a non-error: [object String] at warn (D:\NodeJS\tests\node_modules\bluebird\js\main\debuggability.js:256: 19) at Promise._warn (D:\NodeJS\tests\node_modules\bluebird\js\main\debuggabilit y.js:77:12) at Promise._rejectCallback (D:\NodeJS\tests\node_modules\bluebird\js\main\pr omise.js:411:14) at Function.Promise.reject.Promise.rejected (D:\NodeJS\tests\node_modules\bl uebird\js\main\promise.js:184:9) From previous event: at Promise.longStackTracesCaptureStackTrace as _captureStackTrace at Function.Promise.reject.Promise.rejected (D:\NodeJS\tests\node_modules\bl uebird\js\main\promise.js:183:9) at Object. (D:\NodeJS\tests\test.js:140:13) at Module._compile (module.js:460:26) at Object.Module._extensions..js (module.js:478:10) at Module.load (module.js:355:32) at Function.Module._load (module.js:310:12) at Function.Module.runMain (module.js:501:10) at startup (node.js:129:16) at node.js:814:3 { [AggregateError: aggregate error] '0': 'stop', length: 1 }

— Reply to this email directly or view it on GitHub https://github.com/petkaantonov/bluebird/issues/677.

vitaly-t commented 9 years ago

To output [ 'stop' ], as it happens in other libraries, like When.

benjamingr commented 9 years ago

Promise.any takes an array of promises and resolves when any of those promises resolve (or reject when all of those reject).

Are you looking for Promise.race?

vitaly-t commented 9 years ago

or reject when any of those reject.

That is incorrect. Method any rejects only when all of the promises result in a reject. And when it does, it is supposed to return an array with all rejects, not throw an error.

benjamingr commented 9 years ago

Yes, I meant that .race rejects when any rejects and any rejects when all of them reject.

It's not throwing an error it's rejecting - what you're seeing is two things:

  1. A warning for rejecting with a string, strings should never be rejected with like they should not be thrown. It is allowed because ES6 allowed it but warnings catch it and warn you about it (so you won't lose stack traces and other valuable info).
  2. An AggregateError - which is what .any rejects with errors and lets you enumerate all the errors (this is the "array" you speak of)
vitaly-t commented 9 years ago

Ok, forget the first item, let's change the example:

TEST

promise.any([
    promise.reject(new Error('stop'))
]).
    then(function () {
    }, function (reason) {
        console.log(reason);
    });

OUTPUT

{ [AggregateError: aggregate error] '0': [Error: stop], length: 1 }

This is also unexpected. Why not just return an array of rejects as expected?

benjamingr commented 9 years ago

An AggregateError is an array like object, it contains all the array prototype methods and you can use it like an array. It has better string output than a regular array, you can use it in typed catches and predicate catches better and it can provide more info.

For example, an AggregateError is an Error so it has a stack trace at the correct level (vs an array that is not an error and will give you one level worse of stack traces).

Also - we firmly believe only Errors should be thrown since engines, libraries and code is generally built around that assumption. So it can check for a .stack property and print it.

Imagine I have a logger which does log.write(error.stack.toString()), if the thrown error doesn't have a stack property it's a needless error.

benjamingr commented 9 years ago

Thanks for reporting suspected problems by the way, especially with 3.0 usage. Much appreciated.

vitaly-t commented 9 years ago

Ok, but I was porting a few tests form another promise library, and this part was confusing, because I had these lines in my test:

expect(reason instanceof Array).toBe(true);

which I now had to change to:

expect(reason instanceof Error).toBe(true);

It's just that your approach seems proprietary, not exactly standard or expected. The expected result is what When does in this case.

benjamingr commented 9 years ago

It's just that your approach seems very proprietary, not exactly standard or expected.

It genuinely makes more sense to you to reject with something that is not an error as an error rather than something that has all the advantages of an array (all the methods) + a meaningful stack trace?

The reason Bluebird does this different from When is that we had more time to learn and adapt the API to give more meaningful errors. I'm not sure how it's proprietary, no ES6 promise methods reject with an array either - bluebird is roughly 10 times more popular than When and those are the only two promise libraries I'm aware of that does this.

petkaantonov commented 9 years ago

It's just that your approach seems proprietary, not exactly standard or expected. The expected result is what When does in this case.

Expected according to whom?

Anyway, we (and others) hold the opinion that errors should always be error objects. @benjamingr already explained why but it is also explained on the website. In fact this is what the warning is about when you rejected a promise with a string. Throwing arrays (non-errors) would directly contradict this.

Note that an AggregateError has exactly the same features as an array (when used as read-only), so you are not losing out on anything.