-1

I have code that looks something similar to:

const MyClass {
  checkExists: function(db_client) {
    return new Promise(fulfill, reject) {
      var sql = 'select * from table';
      db_client.connect().then(c => {
      }).then(res => {
        client.release();
        fulfill(res.rows[0].col1 === 1 ? true : false);
      }).catch(error => {
        reject(error);
      });
    });
  }
  doSomething: function(db_client) {
    return new Promise(fulfill, reject) {
      var sql = 'delete from table where x=1';
      db_client.connect().then(c => {     
      }).then(res => {
        fulfill();
      }).catch(error => {
        reject(error);
      });
    });
  }
};

module.exports = MyClass;

var myc = require('./MyClass.js');

myc.checkExists(db_client).then(resp => {
  if(resp === true) {
    myc.doSomething(db_client).then(resp => {
    console.log('success.');
  } else {
    console.log('we are done.');
  }
  }).catch(error => {
    console.log(error);
  });
}).catch(error => {
  console.log(error);
});

Per the example above, I have to run a query that is dependent upon the result of another query. (It's pseudocode, forgive if I made mistakes)

However, I'm noticing that it starts to result in nested Promises or function calls with promises from within the fulfillment of another Promise.

I can see this getting worse and worse. Is this kosher? Is there a better way to think/handle what I'm trying to do?

Edit:

Not sure why it's being marked as a duplicate of a question where the poster seems to be explicitly aware of an anti-pattern and is asking how to avoid it vs. a question where the anti-pattern/solution is not known by the poster, but recognizes an issue in a programming style and is looking for help (in which the discussion may result in the same type of solution).

john
  • 33,520
  • 12
  • 45
  • 62
  • remove the inner catch, move the inner.then out one scope (before the .catch(), and return the inner .doSomething – Kevin B Feb 08 '18 at 22:24
  • 1
    `foo().then(() => bar()).then(() => foobar()).then(allDone).catch(handleError)` – Kevin B Feb 08 '18 at 22:27
  • 2
    Possible duplicate of [What is the explicit promise construction antipattern and how do I avoid it?](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it) – Josh Lee Feb 08 '18 at 22:30

3 Answers3

4

Is there a better way to handle nested promises in NodeJS?

Yes, there is. Lots of improvements you can make. Your pseudo-code seems to be missing some pieces to it, but I'll show the simplest version I can that captures the spirit of what it looks like you were doing:

const MyClass {
    checkExists: function(db_client) {
        var sql = 'select * from table';
        return db_client.connect().then(c => {
            // missing code here in your pseudo code
            // do something with c here to create the variable res
            client.release();
            // make this be the resolved value of the returned promise
            return res.rows[0].col1 === 1;
        });
    }
    doSomething: function(db_client) {
        var sql = 'delete from table where x=1';
        return db_client.connect().then(c => {
            // do something with c here
            // return value or another promise
        });
    }
  };

  module.exports = MyClass;

  var myc = require('./MyClass.js');

  myc.checkExists(db_client).then(resp => {
    if(resp === true) {
        return myc.doSomething(db_client).then(resp => {
            console.log('success.');
            // return something here to be the resolved value of the promise
            return resp;
        });
    } else {
        console.log('we are done.');
        // return something here to be the resolved value of the promise
        return resp;
    }
  }).catch(error => {
    console.log(error);
  });

Here are the basic concepts:

  1. Don't wrap an existing promise with a manually created promise. That's referred to as an anti-pattern and it provides no benefit, balloons your code and is an opportunity for error handling mistakes. Just return the promise you already have.

  2. Chain embedded promises from within a .then() handler by returning the promise from the .then() handler.

  3. You don't need to .catch() at every level unless you explicitly need to do something at the lower level. Rejected promises will automatically propagate up the chain to the next .catch(). This can vastly simplify error handling.

  4. If you need a .catch() at a lower level, even just for logging purposes, then you must throw an error from the .catch() to keep the promise rejected, otherwise the rejection will be considered "handled" and the chain will switch to resolved.

  5. In a .then() handler, you have three options. You can return another promise and it will be added into the chain. You can return a value and it will become the fulfilled value of the parent promise chain. You can throw an exception and the current chain will become rejected. If you return nothing, then the fulfilled value of the parent promise chain becomes undefined (just like a function that returns nothing).

FYI, here are a couple articles on promise anti-patterns which are worth studying and putting into practice:

Six Promise Anti-Patterns

Deferred and .then(success, fail) anti-patterns

Other notes about your code:

  1. In some places it looks like you're missing code to release a database connection.

  2. When designing, you should be very clear about a function that just handles an operation on its own (including errors) and there is no expectation that a caller will handler the error vs. a function whose job it is to carry out an operation and return the result or error so that caller can decide what to do with them. This determines whether you just let the rejected promises get returned (for the caller to deal with) or whether you have to .catch() them here and do something with them.

  3. If you .catch() to clean up or log or something like that, but want the caller to still see the error, you have to re-throw the error so the promise stays rejected after your .catch() runs.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • 2
    Why the downvote? Does this not teach all sorts of things that are relevant to what the OP is doing? Also, a downvote without any explanation does not benefit the community much because the author gets no specific feedback on how to improve their answer. Please leave a comment with reasons when you downvote. The best outcome of a downvote is that the author improves their answer. When you drive-by downvote, there is no opportunity for the best outcome. – jfriend00 Feb 08 '18 at 23:06
  • Success! I understand so much better now. I was totally thinking about this differently and this clears it up for me. – john Feb 08 '18 at 23:20
  • @jfriend00 I won't contest the quality of your answer, however, I find your answer existing on this question in particular to make it not useful. If it were instead presented on one of the more popular duplicates of this it would be far more useful. – Kevin B Feb 08 '18 at 23:55
  • @KevinB - Please point me to a duplicate that teaches ALL the things that are wrong with the OP's code and all the things it looks like they need to learn. You could probably find a quartet of duplicates that might cover everything here in a way not very specific to the OP's code, but how would that be better for anyone? Stack overflow is not better off if you mark something a dup where that dup only covers a portion of what a complete answer could cover. I only mark dups when the dup covers EVERYTHING that needs to be taught in an answer. And, believe me I mark hundreds of dups. – jfriend00 Feb 09 '18 at 00:16
-1

If the function inside a .then returns a value then the result of the .then() becomes a promise that resolves to that value. Using this principle we can re-write one of your methods.

function doSomething(db_client) {
  var sql = 'delete from table where x=1';
  return db_client.connect().then(() => undefined);
}

If you want to return the value of the connect it's even easier

function doSomething(db_client) {
  var sql = 'delete from table where x=1';
  return db_client.connect();
}
macdja38
  • 493
  • 3
  • 16
-1

If you return a promise from within a then(), the next then() of the root promise object, will get the result of that earlier promise. Example

Promise.resolve(42).then(function(answer) {
  return new Promise(function(resolve) {
    setTimeout(function() {
      resolve('Answer to life = ' + answer);
    }, 3000);
  })
}).then(function(str){
    document.write(str);
});
deostroll
  • 11,661
  • 21
  • 90
  • 161