2
\$\begingroup\$

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.");
      }
    })
  }]
\$\endgroup\$
5
  • \$\begingroup\$ Any reason why you aren't using Promises? \$\endgroup\$ – Dan Nov 27 '16 at 2:48
  • \$\begingroup\$ Also your last line has a syntax error with a trailing }]. \$\endgroup\$ – Dan Nov 27 '16 at 2:53
  • \$\begingroup\$ re: Syntax error good catch! \$\endgroup\$ – David Tamrazov Nov 27 '16 at 2:57
  • 1
    \$\begingroup\$ to be honest I'm pretty new to Node development and an experienced friend of mine pointed me in the direction of async so I've been tackling that framework as best I can \$\endgroup\$ – David Tamrazov Nov 27 '16 at 2:58
  • 1
    \$\begingroup\$ Okay. good answer :) \$\endgroup\$ – Dan Nov 27 '16 at 2:58
4
\$\begingroup\$
  • 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.

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.

\$\endgroup\$
4
  • \$\begingroup\$ also be careful with naming variables async, in es7 this will probably break due to async/await \$\endgroup\$ – Dan Nov 27 '16 at 3:20
  • \$\begingroup\$ Wow brilliant, I'm gonna be digging through this response for a while haha. So am I right in understanding that callback functions are aware of the variable in the local scope that they're called in? i.e. you never pass anything to "serializeUser", but the "serializeUser" cb still references fbID \$\endgroup\$ – David Tamrazov Nov 27 '16 at 14:30
  • \$\begingroup\$ Yes, this is a property of closures which JavaScript uses extensively. However, I only did this because that's what your original code did :) @DavidTamrazov \$\endgroup\$ – Dan Nov 27 '16 at 21:25
  • \$\begingroup\$ Oh awesome- then to me it sounds like good practice to export whichever callbacks I need out of their appropriate files (i.e. serializeUser from User.js) instead of defining them within my routes... Either way, thank you very much @Dan Pantry. This is exactly what I need it and I appreciate the patient thoroughness :) \$\endgroup\$ – David Tamrazov Nov 27 '16 at 23:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.