Writing elegant promises in Node.js

Question

I am having a real difficult time writing some clean code with a simple user registration/login manager. I am trying to stay out of nesting/callback hell, and am also not seeing any benefit in using the supposedly cleaner async/await.

Take for example my user registration code. It must perform 3 promises in sequence

1. Ensure the user is unique in the database

2. Hash the password

3. Insert the user into the database

I want to

1. Log any errors in each individual promise call (ie db failure)

2. Log the final result in the highest level function (ie register successful)

3. Send back an HTTP response from the caller.

It looks like this right now

hashPassword(password){
    let self = this;
    return bcrypt.hash(password, 10)
        .then(function(hash) {
            return hash;
        })
        .catch(function(err){
            self.logger.error('failed to hash password');
            self.logger.error(err);
            return Promise.reject(err)
        });
}

insertUser(email, passwordHash){
    let self = this;
    let data = {
        email: email,
        password: passwordHash
    };
    return this.collection.insertOne(data)
        .then(function(res) {
            self.logger.info(`inserted ${email} into db`);
            return res;
        })
        .catch(function(err){
            self.logger.error('failed to add user to db');
            self.logger.error(err);
            return Promise.reject(err)
        });
}

findUser(email){
    let self = this;
    return this.collection.findOne({email:email})
        .then(function(element) {
            if(!element){
                self.logger.info(`user ${email} not found`);
            }
            return element;
        })
        .catch(function(err){
            self.logger.error(`failed to query user ${email}`);
            self.logger.error(err);
            return Promise.reject(err);
        });
}

registerUser(email, password){
    let self = this;
    return this.findUser(email)
        .then(function(element){
            if(element){
                self.logger.error(`user ${email} already exists`);
                // now what do i do here for the caller?
                // reject the promise? resolve with an err code?
                return Promise.reject()
            }
            // what if this one fails? how will the caller know..
            return self.hashPassword(password);
        })        
        .then(function(hash){
            return self.insertUser(email, hash);
        })
        .then(function(){
            self.logger.info(`registered user ${email}`);
        })
        .catch(function(err){
            self.logger.error('failed to register user');
            return Promise.reject(err)
        });
}

I am actually super comfortable with the base functions (findUser, hashPassword, insertUser). But the registerUser barely makes sense. The fact that you return promises within a then call really bothers me, for one, but that's just the way she goes. The real issue here is that, if any of the three calls in the process fails, I'm not sure how to tell my caller which thing failed.

Here is the caller.

router.post('/register', function(req, res, next){
    if(req.body.email && req.body.password && req.body.confirm){
        userManager.registerUser(req.body.email, req.body.password)
            .then(function(result){
                res.sendStatus(200);
            })
            .catch(function(err){
                 res.sendStatus(400);
            });
    }
    else {
        res.sendStatus(400);
    }
});

Now I tried to do everything in async/await, but that just requires lots of nest try/catches which I think are even worse. I am just lost here trying to keep this clean.

Answer 1

I think you're rejecting (ha!) the idea of async/await a bit too quickly; it's exactly what you need for the problem you're having, in terms of simplifying things.

You also can create a custom error (or several...) to make things cleaner to see as well.

For example:

class WrappedError extends Error {
  constructor(previousError, ...params) {
    super(...params);
    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, CustomError);
    }

    this.previousError = previousError;
  }
}

async function hashPassword(password){
   try {
     const hashed = await bcrypt.hash(password, 10);
     return hashed;
   catch (e) {
     throw new WrappedError(e, 'Could not hash the password');
   }
}

async function insertUser(email, passwordHash){
    let data = {
        email: email,
        password: passwordHash
    };
    try {
       const result = await this.collection.insertOne(data);
       return result;
    catch (e) {
       throw new WrappedError(e, `Could not insert ${email} into DB`);
    }
}

async function findUser(email) {
  try {
    const user = await this.collection.findOne({ email });
    return user;
  } catch (e) {
    throw new WrappedError(e, 'Could not find user in the DB');
  }
}

async function registerUser(email, password){
  try {
    let user = await this.findUser(email);
    if (user) {
       const e = new Error(`User ${email} already exists!`);
       throw e;
    } // user doesn't exist, proceed
    const hashed = await this.hashPassword(password);
    user = await insertUser(email, hashed);
    this.logger.info(`inserted ${email} into db`);

    return user;
  catch (e) {
    this.logger.error(e);
  }
}

Arguably, you could either skip most of your functions this way and just call the inner logic directly, handling everything in the RegisterUser function since most of the content is boilerplate around the errors and logging. Alternatively, you could create additional custom errors for each failure case.

Answer 2

You can throw instead of returning Promise.reject

You can replace the callback functions with arrows to reduce noise and avoid the self=this trickery

You could also create some helper functions for the logging.

It would then look like this:

const info = message => value => (this.logger.info(message), value);
const error = message => value => {
    this.logger.error(message);
    throw value;
};

hashPassword(password){
    return bcrypt.hash(password, 10)
        .catch(this.error('failed to hash password'))
}

insertUser(email, password){
    let data = { email, password };

    return this.collection.insertOne(data)
        .then(this.info(`inserted ${email} into db`))
        .catch(this.error('failed to add user to db'));
}

findUser(email){
    return this.collection.findOne({email:email})
        .then(element => {
            if(!element){
                this.logger.info(`user ${email} not found`);
            }
            return element;
        })
        .catch(this.error(`failed to query user ${email}`));
}

registerUser(email, password){
    return this.findUser(email)
        .then(element => {
            if(element){
                this.logger.error(`user ${email} already exists`);
                throw `user ${email} already exists`;
            }
            // if this one fails the last catch in this promise will catch it
            return this.hashPassword(password);
        })        
        .then(hash => this.insertUser(email, hash))
        .then(this.info(`registered user ${email}`))
        .catch(this.error('failed to register user'));
}

Answer 3

Returning rejected promises seems fine to me. Just make sure to reject them with errors which can be understood by the caller. This may include error codes, specialized objects containing detailed data or just plain text. You may also nest errors by rejecting with errors containing a cause property which then contains the deeper reason (yet another error object).

To find the error structure which fits your situation, consider what you would like to receive at the call site.