10

Assume we have an action that runs on user login (express, node). This is the code that works, written using a lot of callbacks:

checkIfEmailAndPasswordAreSet(email, password, (error, response) => {
  if (error) return errorResponse(403, 'validation error', error)
  findUserByEmail(email, (error, user) => {
    if (error) return errorResponse(500, 'db error', error)
    if (!user) return errorResponse(403, 'user not found')
    checkUserPassword(user, password, (error, result) => {
      if (error) return errorResponse(500, 'bcrypt error', error)
      if (!result) return errorResponse(403, 'incorrect password')
      updateUserLastLoggedIn(user, (error, response) => {
        if (error) return errorResponse(500, 'db error', error)      
        generateSessionToken(user, (error, token) => {
          if (error) return errorResponse(500, 'jwt error', error)
          return successResponse(user, token)
        })
      })
    })
  })
})

I want to rewrite this code using async/await and avoid callback hell. How to do that?

The first attempt could look like that:

try {
  await checkIfEmailAndPasswordAreSet(email, password)
  const user = await findUserByEmail(email)
  if (!user) throw new Error('user not found')
  const result = await checkUserPassword(user, password)
  if (!result) throw new Error('incorrect password')
  await updateUserLastLoggedIn(user)
  const token = await generateSessionToken(user)
  return successResponse(user, token)
} catch (e) {
  // How to handle the error here?
}

I want to keep the proper error handling, that is if the error was thrown in checkUserPassword method, I want the response to contain info about this. What should I write in the catch method?

For example, I could wrap every instruction into it's own try / catch block like that:

try {

  let user, result

  try {
    await checkIfEmailAndPasswordAreSet(email, password)
  } catch (error) {
    throw new Error('This error was thrown in checkIfEmailAndPasswordAreSet')
  }

  try {
    user = await findUserByEmail(email)
  } catch (error) {
    throw new Error('This error was thrown in findUserByEmail')
  }

  if (!user) throw new Error('user not found')

  ...
} catch (error) {
  return errorResponse(error)
}

But this code.. probably that's not a callback hell, but I would call it try/catch hell. It takes at least 2 times more rows that the original old fashioned code with callbacks. How to rewrite it to be shorter and take an advantage of async/await?

Kasheftin
  • 7,509
  • 11
  • 41
  • 68
  • 2
    You can add specific error types , like `throw new UserNotFoundError` instead of `new Error('user not found')` and then do some `instanceof` branching in the catch block. – georg Jan 27 '18 at 13:01
  • 1
    `it takes at least 2 more rows` ... cool. But its definetly more readable. – Jonas Wilms Jan 27 '18 at 13:13

1 Answers1

14

There is no easy handling of individual errors with try/catch.

I would probably write some helper functions to facilitate simple error throwing:

function error(code, msg) {
    return e => {
        e.status = code;
        e.details = msg;
        throw e;
    };
}
function ifEmpty(fn) {
    return o => o || fn(new Error("empty"));
}

and then use standard promise methods:

try {
    await checkIfEmailAndPasswordAreSet(email, password)
        .catch(error(403, 'validation error'));
    const user = await findUserByEmail(email)
        .then(ifEmpty(error(403, 'user not found')), error(500, 'db error', error));
    await checkUserPassword(user, password)
        .then(ifEmpty(error(403, 'incorrect password')), error(500, 'bcrypt error'));
    await updateUserLastLoggedIn(user)
        .catch(error(500, 'db error'));
    const token = generateSessionToken(user)
        .catch(error(500, 'jwt error'));
    return successResponse(user, token);
} catch(err) {
    errorResponse(err.status, err.details, err);
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thanks for the idea. I used your approach and the code looks prettier now, imho. Will mark as answer in several days if no other response. – Kasheftin Jan 28 '18 at 11:35
  • 1
    Yeah, prettiness is in the eye of the beholder :-) You might also write different helper functions that operate on promises and could be called like `await rejectEmpty(wrapRejection(checkPassword(…), 500, 'bcrypt'), 403, 'password')`. Any abstraction that makes the code DRY will do :-) – Bergi Jan 28 '18 at 11:57