7

My Promise issue

I am new to Promises and I've been reading the Q Documentation, where it says:

When you get to the end of a chain of promises, you should either return the last promise or end the chain.

I have defined a Promise in my code the Q.Promise way, with the following console.logs to log out an execution trace:

function foo(){
   return Q.Promise(function(resolve, reject) {

    doSomething()
    .then(function() {
      console.log('1');
      return doSomething1();
    })
    .then(function() {
      console.log('2');
      return doSomething2();
    })
    .then(function() {
      console.log('3');
      return doSomething3();
    })
    .catch(function(err) {
      console.log('catch!!');
      reject(err);
    })
    .done(function() {
      console.log('done!!');
      resolve();
    });

  });
}

In case every doSomethingN() executes correctly, everything works as intended and I get the expected trace:

1
2
3
done!!

But in case any of the doSomethingN() fails:

foo() works correctly, because the error function callback is the one that runs whenever a reject(err) occurs:

foo().then(function() { /* */ }, function(err) { /* this runs! */ });

And I get the following trace (ie. when doSomething1() fails):

1
catch!!
done!!

My question

What I thought at first was the following:

Okay, let's handle the chaining success and failure in both: .done() and .catch() methods. If everything goes well .done()'s callback will be executed and the promise will be resolved. In case there's an error at any point, .catch()'s callback will be executed and the promise will be rejected - and because of that, done() won't be executed.

I think I am missing something about how the .done() works... because by having a look at my logging trace, I realized that .done() seems to be executing always - whether there is an error and .catch() is executed or not - and that is what I wasn't expecting.

So, after that, I removed .done()'s callback and now foo():

  • works if there's an error during the chain execution
  • does not work if everything works correctly

What should I reconsider and how could/should I make it work?

charliebrownie
  • 5,777
  • 10
  • 35
  • 55
  • 2
    First of all, avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572)! – Bergi Nov 26 '15 at 21:46
  • 1
    Your problem is not with `done`, but with `catch`. Catching the error means you've handled it, and the promise that is returned from `.catch(…)` - the one you attach `.done()` to - is going to be fulfilled! – Bergi Nov 26 '15 at 21:48
  • @Bergi I'm afraid I don't understand what you are trying to explain in your second comment... What's wrong with that `.catch()`? – charliebrownie Nov 26 '15 at 22:09
  • 1
    It returns a promise that will eventually be fulfilled, which is the reason why your `done` runs even if there was an error. Have a look at [Chained promises not passing on rejection](http://stackoverflow.com/q/16371129/1048572) – Bergi Nov 26 '15 at 22:13
  • @Bergi the link's you've given about the topic are very useful! – charliebrownie Nov 27 '15 at 00:46

3 Answers3

5

catch(cb) is just an alias for then(null, cb), and you've actually fixed an error in catch, so flow naturally turned to success result in done.

If you want to just decorate the error in catch, you should rethrow the error afterwards, e.g. proper passthru may look as:

catch(function (err) {
   console.log(err);
   throw err;
});

Still your example doesn't make much sense. You should never use done, when you return a promise. If you want to resolve initialized promise with internally created chain of promises, you should just resolve it as:

resolve(doSomething()
  .then(function() {
    console.log('1');
    return doSomething1();
  })
  ....
  .then(function() {
    console.log('N');
    return doSomethingN();
  }));

There's no need for internal error handling, leave that to consumer of promise which you return.

And other point. If when creating new promise you know it will be resolved with other one, then there's no logical reason to create such promise, just reuse one you planned to resolve with. Such error was also coined as deferred anti-pattern

Mariusz Nowak
  • 32,050
  • 5
  • 35
  • 37
3

You should consider doing this:

function foo() {
  // Calling .then() on a promise still return a promise.
  // You don't need Q.Promise here
  return doSomething()
    .then(function(doSomethingResult) {
      console.log('1');
      return doSomething1();
    })
    .then(function(doSomething1Result) {
      console.log('2');
      return doSomething2();
    })
    .then(function(doSomething2Result) {
      console.log('3');
      return doSomething3();
    });
}



foo()
  .then(function(fooResult) {
    console.log(fooResult); // fooResult should be what is returned by doSomething3()
  })
  .catch(function(err) {
    console.error(err); // Can be thrown by any 
  })
  .done(function() {
    console.log('I am always executed! error or success');
  });

If you want to return a promise, in most cases it does not make much sense to use catch (unless you want to recover potential errors). It never make sense to use done in a method returning a promise. You would rather use these methods at the very end of the chain.

Notice that doSomethingX() can return either a value, or a promise, it will work the same.

Sebastien Lorber
  • 89,644
  • 67
  • 288
  • 419
  • Nice point out and very neat explained, @SebastienLorber! I have one very last question about where to use `deferreds` or `new Promises`: so, I only should be using these (one or another) to do the "setting" inside the function I want a promise to be returned. In this case, inside `foo()` you don't need `deferreds` or `new Promises` because that was already done in every `doSomethingN()` (which already return a promise) and you get these directly by `.then()`... am I right? I think I'm getting how this works...! – charliebrownie Nov 27 '15 at 00:59
  • and @SebastienLorber, if any of the `doSomethingN()` function threw a `reject`... it would propagate and would be caught at the `foo().catch()` wouldn't it? – charliebrownie Nov 27 '15 at 02:03
  • 1
    Also @charliebrownie if your `doSomethingX()` operations do not need to use the previous results to be performed, you can launch them in parallel (using `Q.all([smth1Promise,smth2Promise,smth3Promise]).spread(res1,res2,res3)`... – Sebastien Lorber Nov 27 '15 at 09:48
0

You can make it work by resolving promise in your last then callback.

function foo(){
    return doSomething()
    .then(function() {
      console.log('1');
      return doSomething1();
    })
    .then(function() {
      console.log('2');
      return doSomething2();
    })
    .then(function() {
      console.log('3');
      return doSomething3();
    })
}

Consider using bluebird for promises. It has many useful features as compared to any other promise library. You may find it difficult to begin it, but once you get hold of it you're going to love it.

Kunal Kapadia
  • 3,223
  • 2
  • 28
  • 36