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));