3 ms·
The problem isn't mixins Promises and callbacks, in fact mixing the two is often completely necessary and people need to learn to do it correctly exactly to avo
by spankalee 10y ago
The problem isn't mixins Promises and callbacks, in fact mixing the two is often completely necessary and people need to learn to do it correctly exactly to avoid problems like these.
The problem here is that chained Promise reactions, like .then(A).catch(B) can indeed call both A and B if A throws, which in this case is very bad if you can only call one of them.
The correct solution is to use an error handler in .then():
function fetchFirstUser(iswhatIwant, callback) {
knex.first().from("users").then((user) => {
callback(user);
}, (e) => {
callback(null);
});
}
But anytime you're invoking callbacks you also have to consider the case where the callback throws - which is one reason returning Promises is generally easier to get correct than invoking callbacks - so you _also_ need a .catch():
function fetchFirstUser(iswhatIwant, callback) {
knex.first().from("users").then((user) => {
callback(user);
}, (e) => {
callback(null);
}).catch((e) => {
// oh no, callback() threw! bad callback!
console.error(e);
});
}
This might seem like a lot, but I blame callbacks in general for the extra work that needs to be done. If you can avoid callbacks, do, because Promises all the way through would be so much simpler:
function fetchFirstUser(iswhatIwant, callback) {
return knex.first().from("users");
}
And push error handling out to the initiator.
- ascagnel_ 10y agoIf you must call the callback, you could simplify this pattern as such: function fetchFirstUser(isWhatIWant, callback) { knex.first().from('users') .then(user => ({ user })) .catch(e => ({ e })) .then(({ user, e }) => { if (user) { callback(user); } else { callback(e); } }); } By only calling your callback function after a catch statement, you guarantee that (a) an exception generated by the callback will not be swallowed by the catch statement and that (b) the callback is called exactly once in the promise chain.
- Vevolut 10y agoI may be mistaken but I don't think this will work exactly how you expect it to. If theres an unhandled exception thrown somewhere in the callback then it will fail silently. You need to add a .catch(console.error.bind(console)); at the very end to get back to default behavior.
- spankalee 10y agoI like this! For golfing purposes, you can remove a branch too: function fetchFirstUser(isWhatIWant, callback) { knex.first().from('users') .then((user) => ({ user }), (e) => ({ e })) .then(({ user, e }) => callback(e, user)) .catch((e) => console.error(e)); }