I'm using Node.js and Bluebird to create some fairly plicated logic involving unpressing a structured file, parsing JSON, creating and making changes to several MongoDB documents, and writing related files in multiple locations. I also have fairly plicated error handling for all of this depending on the state of the system when an error occurs.

I am having difficulty thinking of a good way to manage dependencies through the flow of promises.

My existing code basically looks like this:

var doStuff = function () {
  var dependency1 = null;
  var dependency2 = null;

  promise1()
  .then(function (value) {
    dependency1 = value;

    return promise2()
    .then(function (value) {
      dependency2 = value;

      return promise3(dependency1)
      .then(successFunction);
    });
  })
  .catch(function (err) {
    cleanupDependingOnSystemState(err, dependency1, dependency2);
  });
};

Note that dependency1 isn't needed until promise3, and that the error handler needs to know about the dependencies.

To me this seems like spaghetti code (and my actual code is far worse with a lot of parallel control flow). I've also read that returning another promise inside of a .then callback is an antipattern. Is there a better/cleaner way of acplishing what I'm trying to do?

I'm using Node.js and Bluebird to create some fairly plicated logic involving unpressing a structured file, parsing JSON, creating and making changes to several MongoDB documents, and writing related files in multiple locations. I also have fairly plicated error handling for all of this depending on the state of the system when an error occurs.

I am having difficulty thinking of a good way to manage dependencies through the flow of promises.

My existing code basically looks like this:

var doStuff = function () {
  var dependency1 = null;
  var dependency2 = null;

  promise1()
  .then(function (value) {
    dependency1 = value;

    return promise2()
    .then(function (value) {
      dependency2 = value;

      return promise3(dependency1)
      .then(successFunction);
    });
  })
  .catch(function (err) {
    cleanupDependingOnSystemState(err, dependency1, dependency2);
  });
};

Note that dependency1 isn't needed until promise3, and that the error handler needs to know about the dependencies.

To me this seems like spaghetti code (and my actual code is far worse with a lot of parallel control flow). I've also read that returning another promise inside of a .then callback is an antipattern. Is there a better/cleaner way of acplishing what I'm trying to do?

• javascript
• node.js
• promise
• bluebird

• This is probably better for programmers.stackexchange. to be honest man. – 1252748 Commented Jan 22, 2015 at 2:20
• Does promise2 depend on promise1 having finished? The code implies that, but it's a bit unclear. – loganfsmyth Commented Jan 22, 2015 at 2:38
• Yes, any promise that es later depends on something an earlier promise did/data it retrieved. – Tom O'Connell Commented Jan 22, 2015 at 2:41

3 Answers 3

I find both answers currently provided nice but clumsy. They're both good but contain overhead I don't think you need to have. If you instead use promises as proxies you get a lot of things for free.

var doStuff = function () {
  var p1 = promise1();
  var p2 = p1.then(promise2);
  var p3 = p1.then(promise3); // if you actually need to wait for p2 here, do.
  return Promise.all([p1, p2, p3]).catch(function(err){
      // clean up based on err and state, can unwrap promises here
  });
};

Please do not use successFunction and such it is an anti-pattern and loses information. If you feel like you have to use successFunction you can write:

var doStuff = function () {
  var p1 = promise1();
  var p2 = p1.then(promise2);
  var p3 = p1.then(promise3); // if you actually need to wait for p2 here, do.
  Promise.join(p1, p2, p3, successFunction).catch(function(err){
      // clean up based on err and state, can unwrap promises here
  });
};

However, it is infinitely worse since it won't let the consumer handle errors they may be able to handle.

This question might be more appropriate for code review but here is how I'd approach it given this example:

var doStuff = function () {
  // Set up your promises based on their dependencies. In your example
  // promise2 does not use dependency1 so I left them unrelated.
  var dep1Promise = promise1();
  var dep2Promise = promise2();
  var dep3Promise = dependency1Promise.then(function(value){
    return promise3(value);
  });

  // Wait for all the promises the either succeed or error.
  allResolved([dep1Promise, dep2Promise, dep3Promise])
      .spread(function(dep1, dep2, dep3){

    var err = dep1.error || dep2.error || dep3.error;
    if (err){
      // If any errored, call the function you prescribed
      cleanupDependingOnSystemState(err, dep1.value, dep2.value);
    } else {
      // Call the success handler.
      successFunction(dep3.value);
    }
};

// Promise.all by default just fails on the first error, but since
// you want to pass any partial results to cleanupDependingOnSystemState,
// I added this helper.
function allResolved(promises){
  return Promise.all(promises.map(function(promise){
    return promise.then(function(value){
      return {value: value};
    }, function(err){
      return {error: err};
    });
  });
}

The use of allResolved is only because of your callback specifics, if you had a more general error handler, you could simply resolve using Promise.all directly, or even:

var doStuff = function () {
  // Set up your promises based on their dependencies. In your example
  // promise2 does not use dependency1 so I left them unrelated.
  var dep1Promise = promise1();
  var dep2Promise = promise2();
  var dep3Promise = dependency1Promise.then(function(value){
    return promise3(value);
  });

  dep3Promise.then(successFunction, cleanupDependingOnSystemState);
};

It is certainly not an antipattern to return promises within thens, flattening nested promises is a feature of the promise spec.

Here's a possible rewrite, though I'm not sure it's cleaner:

var doStuff = function () {

  promise1()
  .then(function (value1) {

    return promise2()
    .then(function (value2) {

      return promise3(value1)
      .then(successFunction)
      .finally(function() {
        cleanup(null, value1, value2);
      });

    })
    .finally(function() {
      cleanup(null, value1, null);
    });

  })
  .finally(function () {
    cleanup(null, null, null);
  });

};

Or another option, with atomic cleanup functions:

var doStuff = function () {

  promise1()
  .then(function (value1) {

    return promise2()
    .then(function (value2) {

      return promise3(value1)
      .then(successFunction)
      .finally(function() {
        cleanup3(value2);
      });

    })
    .finally(function() {
      cleanup2(value1);
    });

  })
  .finally(function (err) {
    cleanup1(err);
  });

};

Really, I feel like there's not much you can do to clean this up. Event with vanilla try/catches, the best possible pattern is pretty similar to these.