Javascript Promise Pattern - differentiate error

Question

Consider this block of code:

    getUser(userId)
    .catch(function(error){
        crashreporter.reportError('User DB failed', error);
        // show user a generic error
    })
    .then(function(user) {
        return chargeCreditCard(user);
    })
    .catch(function(error){
        crashreporter.reportError('Credit card failed', error);
        // show user an error saying that their credit card got rejected
    })

Obviously, the problem with this is that the THEN(USER) block gets executed if User DB fails. Another option is to move the first catch block to the end of the chain. However, that causes another issue! We won't be able to differentiate whether the error comes from the User DB or CreditCard.

Is the following pattern, which I think solves the problem, considered a Promise Anti Pattern? Is there a better way of doing this? The problem that I see with this, is that you can end up in a semi-callback hell.

    getUser(userId)
    .then(function(user) {
        return chargeCreditCard(user)
              .catch(function(error){
                  crashreporter.reportError('Credit card failed', error);
                  // show user an error saying that their credit card got rejected
              });
    })
    .catch(function(error){
        crashreporter.reportError('User DB failed', error);
        // show user a generic error
    })

Edit: I guess I haven't been very clear. What if there are more THEN blocks, like what below. The problem is that once you hit one ERROR, you don't want the chain to continue at all.

getUser(userId)
    .then(function(user) {
        return chargeCreditCard(user);
    }, function(error){
        crashreporter.reportError('User DB failed', error);
        // show user a error 1
    })
    .then(function(chargeId) {
        return saveChargeId(chargeId);
    }, function(error){
        crashreporter.reportError('ChargeId DB failed', error);
        // show user a error 2
    })
    .then(function(chargeHistoryId) {
        return associateChargeToUsers(chargeHistoryId);
    }, function(error){
        crashreporter.reportError('chargeHistoryId DB failed', error);
        // show user a error 3
    })
    .catch(function(error){
        crashreporter.reportError('Credit card failed', error);
        // show user a error 4
    })

You might want to also have a look at [How to break promise chain on error](http://stackoverflow.com/q/34748350/1048572), [How to properly break out of a promise chain?](http://stackoverflow.com/q/29499582/1048572) and [Proper way to skip a then function in promises](http://stackoverflow.com/q/21576862/1048572) — Bergi, Feb 03 '17 at 01:41
And [How to return from a Promise's catch/then block](http://stackoverflow.com/a/32049994/3478010). — Roamer-1888, Feb 03 '17 at 05:53
related: [How to normalize error objects in a promise chain?](https://stackoverflow.com/q/39698365/1048572) — Bergi, Sep 07 '19 at 16:51

score 4 · Accepted Answer · edited May 23 '17 at 11:53

Is the following pattern, which I think solves the problem, considered a Promise Anti Pattern?

No, it's fine.

Is there a better way of doing this?

Yes, have a look at the difference between .then(…).catch(…) and .then(…, …). If you want to strictly distinguish the success case (continue) from the error case (report this specific problem), passing two callbacks to then is a better idea. That way, the outer handler cannot be triggered by a failure from the success case code, only from failures in the promise it was installed on. In your case:

getUser(userId)
.then(function(user) {
    return chargeCreditCard(user)
    .then(function(chargeId) {
        return saveChargeId(chargeId)
        .then(function(chargeHistoryId) {
            return associateChargeToUsers(chargeHistoryId);
            .then(function(result) {
                return finalFormatting(result);
            }, function(error){
                crashreporter.reportError('chargeHistoryId DB failed', error);
                return 3;
            });
        }, function(error){
            crashreporter.reportError('ChargeId DB failed', error);
            return 2;
        });
    }, function(error){
        crashreporter.reportError('Credit card failed', error);
        return 4;
    });
}, function(error){
    crashreporter.reportError('User DB failed', error);
    return 1;
})
.then(showToUser);

Though you might want to use a generic error handler:

getUser(userId)
.catch(function(error){
    crashreporter.reportError('User DB failed', error);
    throw new Error(1);
})
.then(function(user) {
    return chargeCreditCard(user)
    .catch(function(error){
        crashreporter.reportError('Credit card failed', error);
        throw new Error(4);
    });
})
.then(function(chargeId) {
    return saveChargeId(chargeId);
    .catch(function(error){
        crashreporter.reportError('ChargeId DB failed', error);
        throw new Error(2);
    });
})
.then(function(chargeHistoryId) {
    return associateChargeToUsers(chargeHistoryId);
    .catch(function(error){
        crashreporter.reportError('chargeHistoryId DB failed', error);
        throw new Error(3);
    });
})
.then(function(result) {
    return finalFormatting(result);
}, function(error) {
    return error.message;
})
.then(showToUser);

Here, every then callback does return a promise that rejects with the appropriate error on its own. Ideally every of the called functions already did that, when they don't and you need to append a specific catch to each of them you might want to use a wrapper helper function (maybe as part of the crashreporter?).

function withCrashReporting(fn, msg, n) {
    return function(x) {
        return fn(x)
        .catch(function(error){
            crashreporter.reportError(msg, error);
            throw new Error(n);
        });
    };
}
withCrashReporting(getUser, 'User DB failed', 1)(userId)
.then(withCrashReporting(chargeCreditCard, 'Credit card failed', 4))
.then(withCrashReporting(saveChargeId, 'ChargeId DB failed', 2))
.then(withCrashReporting(associateChargeToUsers, 'chargeHistoryId DB failed', 3))
.then(finalFormatting, function(error) {
    return error.message;
})
.then(showToUser);

The problem that I see with this, is that you can end up in a semi-callback hell.

Nah, it's just an appropriate level of wrapping. In contrast to callback hell, it can be flattened down to a maximum nesting of two, and it always has a return value.

If you absolutely want to avoid nesting and callbacks, use async/await, though that's actually even uglier:

try {
    var user = await getUser(userId);
} catch(error) {
    crashreporter.reportError('User DB failed', error);
    return showToUser(1);
}
try {
    var chargeId = chargeCreditCard(user);
} catch(error) {
    crashreporter.reportError('Credit card failed', error);
    return showToUser(4);
}
try {
    var chargeHistoryId = saveChargeId(chargeId);
} catch(error) {
    crashreporter.reportError('ChargeId DB failed', error);
    return showToUser(2);
}
try {
    var result = associateChargeToUsers(chargeHistoryId);
} catch(error) {
    crashreporter.reportError('chargeHistoryId DB failed', error);
    return showToUser(3);
}
return showToUser(finalFormatting(result));

I disagree that it's fine. If there was any anti-pattern for promises, it's that. They exist for the purpose of avoiding that sort of callback nesting AKA "callback hell" — Charlie Martin, Feb 03 '17 at 01:47
@CharlieMartin It's all about control flow. If you want different paths through your code (branching), and "not continuing the chain" is exactly that, you need nesting of control structures. And `then` with two callbacks *is* a control structure here. The only way to avoid the nesting would be *early returns* used with `async`/`await`, but I don't think that's much better. — Bergi, Feb 03 '17 at 02:05
It's not the only way. You throw a custom error and then in the next error callback, you say "is this a custom error from above? throw it and continue forwarding down the chain. If not, handle it here". You can't stop the chain, but you can behave differently according to what was passed down to you. Effectively "cancelling" the chain by doing nothing when you receive a custom error. Everything can be done with a single chain. That's the beauty of promises — Charlie Martin, Feb 03 '17 at 02:08
I don't know if that made sense but check the error type and do conditional logic accordingly in the next error callback. That logic might mean returning a new promise and shifting back to the success track. It might mean doing nothing but re-throwing the same error (effectively "cancelling" the chain) if yo do this the rest of the way. Or it might mean throwing a different kind of error. It might mean adding one error to an array of errors and throwing the array to handle them all at once at the end. Wish I could put a code sample here. — Charlie Martin, Feb 03 '17 at 02:17
You think it's beautiful, I find that ugly. Creating so many promises is pretty inefficient, and having to check everywhere whether the error is a custom, wrapped one already or coming from the last function is pretty awful. Chaining `catch()` calls is the equivalent to nesting `try`-`catch` blocks - not what one should do and not what we actually want either. — Bergi, Feb 03 '17 at 02:40
Not sure what you mean. You don't create any promises you didn't already have to. You can abstract the conditional logic out into functions and are left with a single chain with callbacks with one or two line bodies max. It's declarative and powerful. Promises wouldn't exist if it were impossible to avoid nesting them. They exist to solve the nesting problem. — Charlie Martin, Feb 03 '17 at 03:02
Check the difference between the two examples in my answer. In the first pattern, all the inner promises are created conditionally only - if `getUser` succeeds. When it fails, there are only three promises created (and every of them does some real work). In the second example, it always creates at least 7 promises, and when `getUser` fails, it has to bubble the error down that chain without any actual work. Yes, it works, but it's horribly inefficient. — Bergi, Feb 03 '17 at 03:06
The point of the nesting is exactly to have the conditional logic written out declaratively. You can think of every `then` with two callbacks as an `if` statement with then and else clauses for success and error case. No jumping around, no "exceptions" or early returns, just plain structured programming. And it still has a return value, continuing the chain with `showToUser`. This is not one of the cases that promises were meant to flatten. It's possible, but not necessary - both approaches are viable and have their merits. — Bergi, Feb 03 '17 at 03:08
I updated my answer to demonstrate what I mean. Sure, the differences are subtle and a manner of personal preference. Just pointing out that you can always have a flat chain. While we solved callback hell, the general discontent with this pattern has led us to a much nicer solution anyway, which is `async/await` or generators — Charlie Martin, Feb 03 '17 at 03:49
@CharlieMartin Yeah, that's what I guessed. However, when it comes to error handling I actually find `then` a much better (declarative and functional) style than `try`-`catch` blocks in `async function`s. — Bergi, Feb 03 '17 at 03:54
A couple months later, I think your 3rd example is the best we can do +1. I'd like to think I helped you get there though :) — Charlie Martin, Mar 17 '17 at 05:18

Charlie Martin · Answer 2 · 2017-02-03T18:44:19.737

0

Here is how you can accomplish "cancelling" the chain without ever nesting callbacks. This is why promises are a solution to "callback hell"...

var errors = {
    USER: 'User DB failed',
    CHARGE: 'ChargeId DB failed',
    HISTORY: 'chargeHistoryId DB failed',
};

function onError(type, error) {
    throw {
         message: type,
         error: error
    }
}

getUser(userId)
  .then(chargeCreditCard, onError.bind(null, errors.USER))
  .then(saveChargeId, function (error) {
    if (error.message === errors.USER) throw error
    else onError(errors.CHARGE, error);
  })
  .then(associateChargeToUsers, function(error) {
    if (error.message === errors.CHARGE || error.message === errors.USER) throw error
    else onError(errors.HISTORY, error);
  })
  .then(finalFormatting, function(error) {
    crashreporter.reportError(error.message, error.error); 
  })
  .then(showToUser);

Essentially, if the first promise fails, you pass its error down the chain of failure callbacks and when it reaches the end, where it gets logged. No other promises are created, so you have effectively "cancelled" the operation on first failure.

edited Feb 03 '17 at 18:44

answered Feb 03 '17 at 01:22

Charlie Martin

8,208
3
35
41

Yes, I understand that. I guess I haven't been very clear in my question. I have updated my questions – Kasra Feb 03 '17 at 01:34
This is exactly why I consider this pattern error-prone and ugly: your code doesn't work when `getUser()` fails, instead of throwing a `'User DB failed'` error as it should it throws a `'chargeHistoryId DB failed'` error. If you want a flat chain, you should do it like in the second snippet in my answer. – Bergi Feb 03 '17 at 03:51
I missed that little `||` statement. I just typed it off the top of my head. Check it now. It works perfectly. Ugly is your personal preference. Promises are to solve callback hell. Your answer is full of callback hell. If you want a pretty solution, use async/await or generators – Charlie Martin Feb 03 '17 at 03:55
Yeah, but it is *easy* to miss, and a craze of duplicate code, having to repeat the error messages over and over. My answer might be full of callbacks, but it's not hellish. And I believe might third snippet is superior to all the other solutions in every regard. Not even async/await is that pretty or concise. – Bergi Feb 03 '17 at 06:07
It could obviously be refactored to avoid the error message duplication. Arrow functions would help as well. The simplicity of the single level of depth is still preferable for me. Everything else is just tabs vs spaces – Charlie Martin Feb 03 '17 at 06:47
I did some refactoring just for you @Bergi – Charlie Martin Feb 03 '17 at 18:43

score 0 · Answer 3 · answered Feb 03 '17 at 01:26

You should just have one catch for your chain, but you can add more context to the errors that are thrown in each function. For example, when an error condition occurs in chargeCreditCard you could tack onto the error a message property corresponding to what you want to report. Then in your catch error handler you can pass that message property into the reporter:

getUser(userId)
  .then(chargeCreditCard)
  .catch(reportError);

function reportError(error) {
  crashreporter.reportError(error.message, error);
}

Javascript Promise Pattern - differentiate error

3 Answers3

Linked