Cascading promises

Question

Anything beyond a simple promise usually has me perplexed. In this case I need to do 2 asynch calls in a row on N number of objects. First, I need to load a file from disk, then upload that file to a mail server. I prefer to do the two actions together, but I have gotten it working by doing all the reads first and all the uploads second. The code below works, but I can't help but think it can be done better. One thing I don't understand is why the when.all doesn't reject. My interpretation of the docs seems to imply that if one of the promises rejects, the .all will reject. I've commented out the lower resolves in order to test errors. With no errors things seem to work fine and make sense.

mail_sendOne({
  from: 'greg@', 
  to: 'wilma@', 
  subject: 'F&B data', 
  attachments: [
    {name: 'fred.html', path: '/fred.html'}, 
    {name: 'barney.html', path: '/barney.html'}
  ]
})
.done(
  function(res) {
    console.log(res)
  },
  function(err) {
    console.log('error ', err);
  }
)  

function mail_sendOne(kwargs) {
  var d = when.defer();
  var promises = [], uploadIDs = [], errs = [];

  // loop through each attachment
  for (var f=0,att; f < kwargs.attachments.length; f++) {
    att = kwargs.attachments[f];

    // read the attachment from disk  
    promises.push(readFile(att.path)
    .then(
      function(content) {
        // upload attachment to mail server
        return uploadAttachment({file: att.name, content: content})
        .then(
          function(id) {
            // get back file ID from mail server
            uploadIDs.push(id)
          },
          function(err) {
            errs.push(err)
          }
        )
      },
      function(err) {
        errs.push(err)
      }
    ))
  }

  // why doesn't this reject?
  when.all(promises)
  .then(
    function(res) {
      if (errs.length == 0) {
        kwargs.attachments = uploadIDs.join(';');
        sendEmail(kwargs)
        .done(
          function(res) {
            d.resolve(res);
          },
          function(err) {
            d.reject(err);
          }      
        )
      }
      else {
        d.reject(errs.join(','))
      }
    }
  )

  return d.promise;
}

function readFile(path) {
  var d = when.defer();
  var files = {
    '/fred.html': 'Fred Content',
    '/barney.html': 'Barney Content'
  }

  setTimeout(function() {
    d.reject('Read error');
    //d.resolve(files[path]);
  }, 10);

  return d.promise;
}

function uploadAttachment(obj) {
  var d = when.defer();

  setTimeout(function() {
    d.reject('Upload error');
    //d.resolve(new Date().valueOf());
  }, 10);

  return d.promise;
}

function sendEmail(kwargs) {
  var d = when.defer();

  setTimeout(function(){
    console.log('sending ', kwargs)
  }, 5);

  return d.promise;
}

Answer 1

There are a couple of antipatterns there that are making the code messier than it needs to be. One is using .then inside of a .then callback when you should be chaining them. Another is the deferred antipattern.

First, let's create one function each for reading and uploading, both of which handle their respective error and throw a new error with some more context:

function readAndHandle(att) {
    return readFile(att.path)
        .catch(function (error) {
            throw new Error("Error encountered when reading " + att.path + error);
        });
}

function uploadAndHandle(att, content) {
    return uploadAttachment({file: att.name, content: content})
        .catch(function (error) {
            throw new Error("Error encountered when uploading " + att.path + error);
        });
}

Then, let's combine these two into a function that first reads a file, then uploads it. This function returns a promise:

// returns a promise for an uploaded file ID
function readAndUpload(att) {
    return readAndHandle(att)
        .then(function (content) {
             return uploadAndHandle(att, content);
        });
}

Now you can use .map() to map your array of attachments to an array of promises for file ids:

var uploadedIdsPromise = kwargs.attachments.map(readAndUploadAsync);

And that's what you can pass into when.all(). The .then handler on this will pass an array of IDs to its callback:

return when.all(uploadedIdsPromise)
    .then(function (ids) {
        kwargs.attachments = ids.join(";");
        return sendEmail(kwargs);
    })
    .catch(function (error) {
        // handle error
    });

and that's pretty much the gist of it.

One big thing to note here is that with the exception of one place that modifies the kwargs variable, the promises aren't modifying anything outside of the promise chain. That helps keep the logic clean and modular.

The Deferred Antipattern

Note that there are no d.resolves or d.rejects in the above code. The only time you should ever use deferreds is when you don't already have a promise available (or in a few other special situations). And even then, there are preferred ways to create promises these days.

The when.js API Docs say this:

Note: The use of when.defer is discouraged. In most cases, using when.promise , when.try , or when.lift provides better separation of concerns.

The current recommended way to create a promise from some non-promise asynchronous API is to use the revealing constructor pattern. To take your uploadAttachment() function as an example, it would look like this:

function uploadAttachment(obj) {
  return when.promise(function (resolve, reject) {
      setTimeout(function() {
          resolve(new Date().valueOf());
          // or reject("Upload error");
      }, 10);
  });
}

This is the way the ES6 promise API works, and it saves you from having to shuffle around a deferred object.