ysmood / yaku

A lightweight promise library
https://tonicdev.com/ysmood/yaku
MIT License
291 stars 28 forks source link

Finally swallows rejections #49

Closed aiham closed 7 years ago

aiham commented 7 years ago

If I call finally on a rejected promise it unexpectedly becomes fulfilled.

const Promise = require(process.argv[2]);

const p = new Promise((resolve, reject) => {
  setTimeout(() => reject(new Error('simulated error')));
});

p.finally(() => {
  console.log('Cleanup');
})
.then(result => {
  console.log('Unexpected success', result);
})
.catch(err => {
  console.error('Expected failure', err);
});

With yaku:

$ node index.js yaku
Cleanup
Unexpected success Error: simulated error

With bluebird:

$ node index.js bluebird
Cleanup
Expected failure Error: simulated error

Your finally test doesn't catch this because although you expect 'error' here: https://github.com/ysmood/yaku/blob/master/test/finally.js#L28

You do not confirm that its a rejection here: https://github.com/ysmood/yaku/blob/master/test/testSuit.js#L14

The cause is that you do not rethrow or wrap value with Promise.reject here: https://github.com/ysmood/yaku/blob/b5187ec39a90befed234ad27c980d7a36744ef49/src/yaku.js#L154

aiham commented 7 years ago

I would submit a fix but I don't know how you want to update your custom test framework to also take into account the expected Promise result type.

ysmood commented 7 years ago

Thanks for your report. Sorry that I mistook how finally works.

try {
    try {
        throw 'err'
    } finally {
        console.log('ok')
    }
} catch (e) {
    console.log(e)
}

I thought the console.log(e) won't be executed, but I'm wrong. It's not catch. I'll fix it as soon as possible.

ysmood commented 7 years ago

v0.18.1 published. If there's any problem, I will reopen this issue.

aiham commented 7 years ago

@ysmood Thanks for the quick response!