Converting a nested callback to a Promise

Question

I would like to try and convert this function into a Promise based function in order to resolve all of these nested callbacks and return warnings from ESLint consistent-return.

Previously, I asked for some help in overcoming an ESLint error with my return statements here as they are not consistent or following the best practices of JS.

My first thought was to simply do return new Promise((resolve, reject) => {...}) inside the remove function, but that would just promisify the whole thing instead of just what's inside the function so I feel like that's not the best way to do this.

Any help appreciated!

function remove(req, res) {
  User.findOne({ username: req.params.username }, (findErr, existingUser) => {
    if (findErr) return res.status(500).send(errorHandler.getErrorMessage(findErr));
    if (!existingUser) return res.status(404).send({ message: 'User not found' });

    existingUser.remove((removeErr) => {
      if (removeErr) return res.status(500).send(errorHandler.getErrorMessage(removeErr));

      return res.json({ message: `${existingUser.username} successfully deleted` });
    });
  });
}

Answer 1

Here's another way you can do it. I started by "promisifying" each findOne and removeUser as separate functions. Then, your route is almost automatically simplified.

There's still some improvements you could make here, but perhaps there's something you can learn from this.

(Thanks @Bergi for the useful recommendations)

const error = (type, message) => Object.assign(new Error(message), {type});
const wrapError = type => err => { throw error(type, errorHandler.getErrorMessage(err));};

const findUser = opts => {
  return new Promise((resolve, reject) => {
    User.findOne(opts, (err, user) => {
      if (err) reject(err);
      else resolve(user);
    });
  }).then(user => {
    if (!user) throw error('USER_NOT_FOUND', 'User not found')
    else return user;
  }, wrapError('USER_FIND_ERROR'));
};

const removeUser = user => {
  return new Promise((resolve, reject) => {
    user.remove(err => {
      if (err) reject(err);
      else resolve();
    });
  }).catch(wrapError('USER_REMOVE_ERROR'));
};

function remove(req, res) {
  findUser({ username: req.params.username })
    .then(removeUser)
    .then(() => res.json({message: `${req.params.username} successfully removed`}))
    .catch(error) => {
      switch (error.type) {
        case 'USER_NOT_FOUND':
          return res.status(404).send(error.message);
        // case 'USER_FIND_ERROR':
        // case 'USER_REMOVE_ERROR':
        default:
          console.error(error.type, error.message, error.stack);
          return res.status(500).send(error.message);
      }
    });
}

Answer 2

Not sure if got your point but you may want to try the following

const findUser = (username) => {
  return new Promise((resolve, reject) => {
    User.findOne({ username }, (error, user) => {
      if (error) {
        reject({ type: 'error', details: errorHandler.getErrorMessage(error) });
        return;
      }

      if (!user) {
        reject({ type: 'not-found', details: { message: 'User not found' } });
        return;
      }

      resolve(user);
    });
  });
};

const removeUser = (username) => {
  return new Promise((resolve, reject) => {
    findUser(username)
      .then(user => {
        user.remove((error) => {
          if (error) {
            reject({ type: 'error', details: errorHandler.getErrorMessage(error) });
            return;
          }

          // Simply resolve on success
          resolve();
        });
      })
      .catch(error => reject(error));
  });
};

function remove(req, res) {
  removeUser(req.params.username)
    .then(() => res.json({ message: `${req.params.username} successfully deleted` }))
    .catch(error => {
      if (error.type === 'not-found') {
        return res.status(404).send(error.details);
      }

      return res.status(500).send(error.details);
    });
}

As you might have noticed above, some of the behaviours have been extracted to functions that return Promises.

Could have possibly been optimised even more but I just wanted to show you what's possible with Promises.

Does that help?