Bluebird promises - nesting vs rejecting pattern

Question

I'm working on an app for which we're using promises. I'm attempting to figure out what the better pattern is.

Breaking up concerns between thenables and rejecting the promise if an error. Rejection is then handled in a catch block. Or is it better to throw a new type of error and handle in the catch block?

Account.findOneAsync({email: request.payload.email})
  .then(function (user) {
    if (user) {
      return user.compareHash(request.payload.password);
    } else {
      // Account not found
      return Bpromise.reject('AccountNotFound');
    }
  })
  .then(function (validPassword) {
    if (validPassword) {
      return request.auth.jwt.user.sign();
    } else {
      // Invalid password
      return Bpromise.reject('InvalidPassword');
    }
  })
  .then(function (jwt) {
    var response = reply.success();
    return response.header('authorization', jwt);
  })
  .catch(function (e) {
    if (e === 'AccountNotFound' || e === 'Invalid Password') {
      return reply(Boom.unauthorized('Invalid username/password'));
    } else {
      // Perhaps log something like unhandled error
      return reply(Boom.unauthorized('Invalid username/password'));
    }
  });

Or nesting promising as such. I feel here that this is just going down the same rabbit hole of "callback hell" though.

Account.findOneAsync({email: request.payload.email})
      .then(function (user) {
        if (user) {
          user.compareHash(request.payload.password)
            .then(function (valid) {
              if (valid) {
                request.server.plugins.jwt.sign()
                  .then(function (jwt) {
                    var response = reply.success();
                    return response.header('authorization', jwt);
                  });
              } else {
                // Invalid password
                return reply(Boom.unauthorized('Invalid username/password'));
              }
            });
        } else {
          // Account not found
          return reply(Boom.unauthorized('Invalid username/password'));
        }
      })
      .catch(function (e) {
        console.log(e);
      });

Do you see any benefit in the second approach? The first one looks much better to me. — JLRishe, Mar 15 '15 at 16:47
Well if I go with the first approach, I'm wondering what's better. To throw a new error or to reject the promise. I suppose if I throw an error I would need to subclass Error() to make errors like 'throw new AccountNotFound()' — Patrick Wolf, Mar 15 '15 at 16:52
As I've said in the answer I posted seconds ago, I think the idiomatic thing to do is throw an error. There's no need to subclass Error. The Error constructor takes a message string that can be inspected with `e.message`. — JLRishe, Mar 15 '15 at 16:54
So I now see what you're getting at in the second approach. You are directly translating the invalid outcomes into `Boom.unauthorized`s rather than throwing an error only to change it into a response afterward. I will mull on this... — JLRishe, Mar 15 '15 at 16:58
see also [Handling multiple catches in promise chain](http://stackoverflow.com/q/26076511/1048572) — Bergi, Mar 15 '15 at 17:04
Thanks Bergi. I read through that and it believe it adds a bit of overhead by sublassing Error into its own types for each model. Or maybe that is the best way to do things. https://github.com/petkaantonov/bluebird/wiki/Error:-Catch-filter-must-inherit-from-Error-or-be-a-simple-predicate-function suggests that a function can be used as a predicate instead of a custom error. Maybe I just create a predicate functions that matches specific errors in e.message with a final catch block for unhandled errors. — Patrick Wolf, Mar 15 '15 at 17:20

JLRishe · Accepted Answer · 2015-03-15T17:01:22.997

I think you can get the best of both worlds by throwing and then catching your boom objects.

One thing you're missing in both approaches is that when you're already inside a then handler, the idiomatic thing to do is throw an error rather than creating and returning a rejected promise. You also don't need an else block after a return statement:

Account.findOneAsync({email: request.payload.email})
  .then(function (user) {
    if (user) {
      return user.compareHash(request.payload.password);
    }
    // Account not found
    throw Boom.unauthorized('Invalid username/password');
  })
  .then(function (validPassword) {
    if (validPassword) {
      return request.auth.jwt.user.sign();
    }
    // Invalid password
    throw Boom.unauthorized('Invalid username/password');
  })
  .then(function (jwt) {
    var response = reply.success();
    return response.header('authorization', jwt);
  })
  .catch(function (e) {
    if (e.isBoom) {
      return reply(e);
    }
    // Perhaps log something like unhandled error
    return reply(Boom.unauthorized('Invalid username/password'));
  });

I actually had to end up testing the request.auth.jwt.user.sign() within the first thenable. Mainly for the reason that I needed access to the user object when signing the jwt. Given that the promise spec doesn't allow for returning multiple values I felt it was safer to implement that way. — Patrick Wolf, Mar 16 '15 at 01:13

Bluebird promises - nesting vs rejecting pattern

1 Answers1