Promises with NodeJS and BlueBird

Question

I'm using bluebird for promises in my Node/Express application and wrote an API call in which the user passes in a JSON Web Token that contains their user information, and then I decode the token, and pull up the events that user should see based off their userId.

If anyone could give me advice on how I can clean this code up while also using the proper promise practices, that would be great.

/routes.js

app.post('/api/events/', require('./views/api/index').events); 

/views/api/index.js

var B_Promise = require('bluebird');
var jwt = require('jsonwebtoken');

exports.events = function(req, res) {
  var results = {};
  var errors = [];

  var validateEmptyFields = function() {
    return new B_Promise(function(resolve, reject) {
      var token = req.body.token || req.param('token') || req.headers['x-access-token'];
      if (!token) { 
        return reject('Please provide the token parameter'); 
      }

      resolve(token);
    });
  };

  var getUser = function(token) {
    return new B_Promise(function(resolve, reject) {
      jwt.verify(token, req.app.config.api.secret, function(err, decoded) {
        if (err) { 
          return reject(err); 
        }

        resolve(decoded);
      });
    });
  };

  var getEvents = function(user) {
    return new B_Promise(function(resolve, reject) {
      req.app.db.getConnection(function(err, connection){
        if (err) { 
          return reject(err); 
        }

        /* jshint multistr: true */
        connection.query('SELECT e.* FROM events e \
          INNER JOIN event_to_groups etg ON e.id=etg.event_id \
          INNER JOIN user_to_groups utg ON utg.group_id=etg.group_id \
          WHERE utg.user_id=?', user.id, function(err, events) {
            if (err) { 
              return reject(err); 
            }

            results.events = events;
            resolve();
        });

        connection.release();
      });
    });
  };

  validateEmptyFields()
    .then(getUser)
    .then(getEvents)
    .catch(function(err) {
      errors.push(err);
    })
    .finally(function() {
      res.json({results: results, errors: errors});
    });
};

Answer 1

• I would just name the variable for Bluebird promises as Promise as it mostly acts like the standard Promise object in ES6. That way, if you happen to run an ES6-compatible Node.js, you can just remove the import, and you'll be set.

• In the promise constructor, you need not do a return. The code will also feel awkward to a new developer, thinking Promise needs a return. You can simply do:

  jwt.verify(token, req.app.config.api.secret, function(err, decoded) {
    if (err) reject(err); 
    else resolve(decoded);
  });
  

• Bluebird has a static method called Promise.promisify which converts callback-style APIs into promise-returning ones. Just make sure you follow it's guidelines in that:

  function should conform to node.js convention of accepting a callback as last argument and calling that callback with error as the first argument and success value on the second argument.

• If your operation needs to return a promise, but it's not async, then you can simply return a resolved or rejected promise instantly using Promise.resolve and Promise.reject static methods, respectively.

  var validateEmptyFields = function() {
    var token = req.body.token || req.param('token') || req.headers['x-access-token'];
    return token ? Promise.resolve(token) : Promise.reject('Please provide the token parameter')
  };
  

• In getEvents, I suggest you resolve with events. Then assemble results in the same way you assembled errors in catch. The idea is that your promise-generating functions should not be causing side-effects. It should only be aware of the fact that you called it with some arguments, and it resolves or rejects and nothing more.

• Move out that SQL query from the logic. It's messy.

And so, without further ado:

var Promise = require('bluebird');
var jwt = require('jsonwebtoken');

var EVENT_QUERY = '\
SELECT e.* FROM events e \
INNER JOIN event_to_groups etg ON e.id=etg.event_id \
INNER JOIN user_to_groups utg ON utg.group_id=etg.group_id \
WHERE utg.user_id=?';

exports.events = function(req, res) {
  var results = {};
  var errors = [];

  var validateEmptyFields = function() {
    var token = req.body.token || req.param('token') || req.headers['x-access-token'];
    if (!token) Promise.reject('Please provide the token parameter');
    else Promise.resolve(token);
  };

  var getUser = function(token) {
    return new Promise(function(resolve, reject) {
      jwt.verify(token, req.app.config.api.secret, function(err, decoded) {
        if (err) return reject(err);
        else resolve(decoded);
      });
    });
  };

  var getEvents = function(user) {
    return new Promise(function(resolve, reject) {
      req.app.db.getConnection(function(err, connection) {
        if (err) return reject(err);

        connection.query(EVENT_QUERY, user.id, function(err, events) {
          if (err) return reject(err);
          else resolve(events);
        });

        connection.release();
      });
    });
  };

  validateEmptyFields()
    .then(getUser)
    .then(getEvents)
    .then(function(events) {
      results.events = events;
    }, function(err) {
      errors.push(err);
    })
    .finally(function() {
      res.json({
        results: results,
        errors: errors
      });
    });
};

Answer 2

1. It's typical to see promise libraries given the variable name Promise as others have stated, it (somewhat) decouples your code from the Promise implementation you currently favor.

2. Let's talk about how you are putting objects onto your request object. I know that express and other frameworks give functionality for this in the form of app.locals (it's possible to put almost anything onto the request object), but I suggest you don't. Remember, it is a request object. I can't see why I would ever look for application config or db on a request object. (That being said, I suggest you break db/config/errors out into their own modules. However, my code review won't show this to you.)

3. It may seem picky of me to say this, but correct function names go a long way. Your validateEmptyFields function is not validating empty fields, it's checking for existence then returning the request's token. Your getUser function isn't just getting a user, it's verifying/decoding a jwt, this name would be very misleading if I was someone debugging a stack trace which included the function.

4. There's no reason to assign your functions to vars in most of the cases you do. It's best practice not to do so unless your are creating a function at runtime (as opposed to parse time).

5. Remove string literals where you can. In the case of the reject('Please provide the token parameter'); you should be rejecting with a real Error object, preferably one that you've created a type for in another file. (My code won't show you how to do this, it can be homework)

6. I'm unsure why you want to open/close your db connection often. The typical pattern is to have some db module bootstrapped at the start of your application which creates the connection at that time. (Unfortunately, again, I don't have time to show this, it can be homework)

7. Finally, it's fairly bad practice to have your results/errors object globally defined. Especially since in your case, one of the two will always be undefined when your object is sent back to the client.

Take a look at how I restructured your Promise chain. You can promisify most of the function calls that used callbacks and make the final chain tell a story. var Promise = require('bluebird'); // Renamed from B_Promise -> Promise var jwt = require('jsonwebtoken'); var multiline = require('multiline');

exports.events = function(req, res) {
  // Later on, it would be nice to use a query builder to handle the creation of this string
  // For now, we'll just clean it up with multiline
  var EVENTS_QUERY = multiline(function () {/*
   SELECT e.* FROM events e
   INNER JOIN event_to_groups etg ON e.id=etg.event_id
   INNER JOIN user_to_groups utg ON utg.group_id=etg.group_id
   WHERE utg.user_id=?
  */});

  // Please make this error message into a typed error object elsewhere
  var invalidTokenErrorMsg = 'Please provide the token parameter';

  // Should refactor this out elsewhere so connection isn't handled in this file
  function getDbConnection () {
    return Promise.promisify(req.app.db.getConnection).call();
  }

  function decodeUserToken(token) {
    return Promise.promisify(jwt.verify).call(jwt, token, req.app.config.api.secret);
  }

  // Should really refactor this out into some sort of authn module so jwt isn't handled in this file
  function getJwt() {
    var token = req.body.token || req.param('token') || req.headers['x-access-token'];

    return token ? Promise.reject(new Error(invalidTokenErrorMsg)) : Promise.resolve(token);
  }

  // The only function that should really be handled by an events module is here
  // Just something that manages and executes the queries related to events on the database
  function getEventsByUser(userToken) {
    return getDbConnection()
      .then(function (connection) {
        var promisedQuery = Promise.promisify(connection.query, {context: connection});

        return promisedQuery(EVENTS_QUERY, userToken.id);
      })
    ;
  }

  return getJwt()
    .then(decodeUserToken)
    .then(getEventsByUser)
    .then(function (res) {
      res.json({results: res});
    })
    .catch(function(err) {
      res.json({errors: err});
    })
  ;
};