issue in converting a callback to promise

Question

I'm a beginner trying to understand node.js. I tried to convert a callback into promise in my passport local strategy, after transforming into promise I'm having an error when I try to log in that says "user is not defined". I am sure I have done something not right in transforming my callback function, would you please have a look on my code and explain me where have i done wrong.

here's the previous callback

exports.newLocalStrategy= new localStrategy(
(username,password,done)=>{
  User.find({username: username},(err, user)=>{
    if (err) throw err;
    if(user.length == 0){
      console.log("Unknown User");
      return done(null,false,{message: 'unknown User'});

    }
    comparePassword(password,user[0].password, (err,isMatch)=>{
      if (err) throw err;
      if (isMatch){

        return done(null, user);
        return res.send("Loggedin");

      }else{
        console.log('invalid password');
        return done(null, false, {message:"Invalid password"});
      }
    })
  });
});
const comparePassword = (candidatePassword, hash, callback)=>{
    bcrypt.compare(candidatePassword, hash, (err, isMatch)=>{
        if (err) return callback(err);
        callback(null, isMatch);
      });
}

and the code after I made it into promise:

 exports.newLocalStrategy= new localStrategy(
  (username,password,done)=>{
    //promise
    User.find({username: username})
      .then(user =>{
        if(user.length ==0){
          return done(null, false,{message: 'unknown user'})
        }
      })
      .catch(err => {return done(null,err)})
    comparePassword(password,user[0].password)
      .then(isMatch => {
        if (isMatch) return (done,null);
      })
      .catch(err=>{return done(null,err)})
});
const comparePassword = (candidatePassword, hash, callback)=>{
  return new Promise((resolve,reject)=>{
    bcrypt.compare(candidatePassword, hash, (err, isMatch)=>{
      if (err) return reject(err);
      resolve(null, isMatch);
    });
  })
}

I am getting the error at comparePassword(password,user[0].password) this line

Answer 1

There are two main issues here. Since it seems like you want to continue making the newLocalStrategy accept a done callback, I'll assume you don't want it to return a promise, but rather just use promises internally.

exports.newLocalStrategy = new localStrategy(
  (username, password, done) => {
    User.find({
        username: username
      })
      .then(user => {
        if (user.length == 0) {
          return done(null, false, { message: 'unknown user' })
        }
      })
      .catch(err => {
        return done(null, err)
      })

    // this is in the wrong scope, `user` is not defined here
    comparePassword(password, user[0].password)
      .then(isMatch => {
        if (isMatch) return (done, null);
      })
      .catch(err => {
        return done(null, err)
      })
  });

const comparePassword = (candidatePassword, hash, callback) => {
  return new Promise((resolve, reject) => {
    bcrypt.compare(candidatePassword, hash, (err, isMatch) => {
      if (err) return reject(err);
      // resolve takes only one parameter
      resolve(null, isMatch);
    });
  })
}

After making the corrections, it should look like this:

exports.newLocalStrategy = new localStrategy((username, password, done) => {
  User.find({ username }).then(users => {
    if (users.length === 0) {
      throw new Error('unknown user');
    } else {
      return Promise.all([users, comparePassword(password, users[0].password)]);
    }
  }).then(([users, isMatch]) => {
    if (isMatch) done(null, users);
    else throw new Error('invalid password');
  }).catch(err => {
    done(null, false, err)
  });
});

const comparePassword = (candidatePassword, hash) => {
  return new Promise((resolve, reject) => {
    bcrypt.compare(candidatePassword, hash, (err, isMatch) => {
      if (err) reject(err);
      else resolve(isMatch);
    });
  });
};

The most confusing part of this is probably the line

return Promise.all([users, comparePassword(password, users[0].password)]);

This could have been simplified to

return comparePassword(password, users[0].password);

if the users didn't have to be passed to the next .then(), since it's being passed to done() if there's a match. Promise.all() accepts an array of promises and resolves them before invoking the next .then() callback. users is not a promise but it is implicitly converted to one with Promise.resolve() internally, and passed along with the value resolved from comparePassword().

For future reference, you could have defined comparePassword() using util.promisify():

const comparePassword = require('util').promisify(bcrypt.compare);