Is this a reasonable way to implement promises in node.js?

Question

I'm using passport to setup login in an express site. The site is just a playground for me to learn. The code inside login was exhibiting the arrow anti pattern so I decided it was time to learn to use promises.

But I don't know if what I've arrived at is good, bad, idiomatic, or idiotic. Any and all feedback appreciated!

var passport = require('passport');
var LocalStrategy = require('passport-local').Strategy;
var Promise = require('bluebird');
var bcrypt = require('bcrypt');
var db = require('./db').db; //users are stored in mongo

//I'm using bluebird.js for promises
var users = Promise.promisifyAll(db.users);
var compare = Promise.promisify(bcrypt.compare);

//I think this helps when responding to the db user load completing
function NoMatchedUserError(message) {
  this.name = "NoMatchedUserError";
  this.message = message || "Incorrect username.";
}
NoMatchedUserError.prototype = new Error();
NoMatchedUserError.prototype.constructor = NoMatchedUserError;

//this strategy is used by passport to handle logins
module.exports.localStrategy = new LocalStrategy(function(username, password, done) {
  var matchedUser;

  var comparePassword = function(user){
    if(!user) {
      throw new NoMatchedUserError();
    }

    //memoise the loaded user so it can be returned below
    matchedUser = user;
    return compare(password, matchedUser.password);
  };

  users.findOneAsync({ username: username })
    .then(comparePassword)
    .then(function(isMatch) {
      return isMatch
        ? done(null, matchedUser)
        : done(null, false, { message: 'Incorrect password.' });
    })
    .catch(NoMatchedUserError, function() {
      return done(null, false, { message: 'Incorrect username.' });
    }) 
    .error(function(err) {
      return done(err);
    });
});

Esailija · Accepted Answer · 2014-05-26 09:55:56Z

Define all your error types globally in some file for convenience, like the built-in errors are defined.

Mixing promises and callbacks is a big anti-pattern, just think in promises and let .nodeify handle the mapping to callback equivalent at the end.

var passport = require('passport');
var LocalStrategy = require('passport-local').Strategy;
var Promise = require('bluebird');
var bcrypt = require('bcrypt');
var db = require('./db').db; //users are stored in mongo

//I'm using bluebird.js for promises
var users = Promise.promisifyAll(db.users);
var compare = Promise.promisify(bcrypt.compare);


// This strategy is used by passport to handle logins
module.exports.localStrategy = new LocalStrategy(function(username, password, done) {
  users.findOneAsync({username: username}).bind({})
    .then(function(user) {
        if (!user) {
          throw new NoMatchedUserError('Incorrect username.');
        }
        this.user = user;
        return compare(password, user.password);
    })
    .then(function(isMatch) {
      if (isMatch) {
        return this.user;
      }
      else {
        throw { message: 'Incorrect password.' };
      }
    })
    .nodeify(done);
});

.nodeify is now .asCallback bluebirdjs.com/docs/api/ascallback.html — Richard Ayotte
– Richard Ayotte, Commented Mar 7, 2016 at 16:36

Stack Exchange Network

Is this a reasonable way to implement promises in node.js?

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Is this a reasonable way to implement promises in node.js?

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions