async.waterfall call

Does anyone have any recommendations for how I could make this async.waterfall call neater? Ideally I’d be able to do something like:

async.waterfall([
  serializeUser(user_id), 
  generateAccessToken(user_id), 
  generateRefreshToken(user_id)
], function(error, results) {
  //my user, access token, and refresh token are all in results
});

But as far as I know, I can’t just aggregate all of my results at the end without explicitly doing so throughout my functions array, nor can I just pass in variables like I did there, as each subsequent function call depends on the one prior to it. The code below is functioning just fine and does exactly what it needs to, I simply want to make it cleaner.

    var fb_id = req.body.profile.fbID;

    async.waterfall([
      function(cb) {
        return User.serializeUser(fb_id, cb);
      },
      function(user, cb) {
        return auth.generateAccessToken(user._id, function(error, access_token) {
          return cb(null, user, access_token);
        });
      },
      function(user, access_token, cb) {
        return auth.generateRefreshToken(user._id, function(error, refresh_token) {
          if (error) {
            return cb(error, null);
          }
          else if (refresh_token) {
            return cb(null, {user: user, at: access_token, rt: refresh_token});
          }
          //no error but no token either- no clue what happened, return a code of 500 and say its a server error
          else {
            return cb(null, "An unknown error has occured.", 500);
          }
        });
      }
    ], function(error, results){
      if (err) {
        console.log("Waterfall error: ", error, "n");
        res.send("Error: ", error);
      }
      else if (results) {
        console.log("Waterfall results: ", obj, "n");
        res.status(200).json({ user: results.user,  access_token: results.at.access_token, refresh_token: results.rt.tokenString})
      }
      else {
        res.status(500).send("An unknown error has occured.");
      }
    })
  }]

• Keep your variable naming consistent. Some of your code uses camelCase variable names, others uses snake_case. In JavaScript, we use camelCase by default.

• The first argument in a Node-style callback should be reserved for errors.

You do this fairly well, but at one branch inside generateRefreshToken you pass a string error and error code to the callback as success arguments rather than as an Error. Speaking of which, use Error objects rather than strings for errors.

You can create your own derivations of the Error type if you want to be able to determine more specifically what Error you’ve received. This is generally not necessary unless you want your error to be recoverable.

You should set up your express application such that any Error that bubbles up to the top (That is not an authentication error) is presented to the user as a generic HTTP 500 error with no specific information encoded in it. The easiest way to do this is to throw Errors inside middleware when things go wrong and then have another middleware catch this error and present it to the user. I’d recommend boom for this.

• Consider using Promises for async flow.

You can of course continue to use callbacks in this manner with the async library but Promises are (in my opinion) a lot simpler to use and compose. When you are interacting with a library that primarily uses callbacks you can still use Promises at the edges of that API, but I’d recommend using Promises everywhere internally.

The main benefit is that the code is more readable, but also Promises make it much harder for you to forget to handle an error (it’s very easy to forget that in callbacks), make code more composable and are also first class citizens in ES6 as well as receiving syntactic sugar in ES7.

• Extract the logic inside your async.waterfall code to separate functions

You already alluded to this in your answer, but you can very easily extract this code out so it reads a lot easier.

• Rule of silence

Don’t console.log unless it’s for output to user actions. If you really want to do this, use debug from npm, which enables you to flag all debug actions and have them disabled unless you pass an environment variable.

Errors should be written to console.error as this can be piped to a separate error log file.

---

Here’s my revised version of your code in ES5:

var async = require('async')

function serializeUser(callback) {
  return User.serializeUser(fbId, cb)
}

function generateAccessToken(user, callback) {
  return auth.generateAccessToken(user._id, function (error, accessToken) {
    return callback(null, user, accessToken)
  })
}

function generateRefreshToken(user, accessToken, callback) {
  return auth.generateRefreshToken(user._id, function (error, refreshToken) {
    if (error) {
      return cb(error, null)
    } else if (refreshToken) {
      return cb(null, { user: user, at: accessToken, rt: refreshToken })
    } else {
      const err = new Error('no refresh token was generated')
      return cb(err)
    }
  })
}

function requestHandler(req, res, next) {
  var fb_id = req.body.profile.fbID
  async.waterfall([
    serializeUser,
    generateAccessToken,
    generateRefreshToken
  ], function (error, results) {
    if (error) {
      // Let another middleware handle this (like express-boom).
      throw error
    }

    res.status(200)
      .json({
        user: results.user,
        access_token: results.at.access_token,
        refresh_token: results.rt.tokenString
      })
  })
}

I haven’t included Promises here due to your answer about being new to Node – also, they don’t really work extremely well with express.