6 ms·

Never Mix Promises and Callbacks in NodeJS

65 points by philk10 10y ago

mariusc23 10y ago
Wouldn't using the second parameter of .then() to catch exceptions work instead of .catch()? http://stackoverflow.com/a/33278420 http://stackoverflow.com/a/33278420
- pornel 10y ago
  It doesn't matter. The problem is that `callback(null)` throws, and in the `Promise` land this will never result in an uncaught exception. An exception directly inside any `Promise` callback produces another `Promise` in the chain instead. You could at best use `setTimeout` or `nextTick` to escape from `Promise`'s call stack and throw there.
  - mariusc23 10y ago
    Good point! I thought of solving the callback getting called twice problem and didn't think of the bigger picture that mocha needs to catch the assertion error. I like the setTimeout/nextTick trick (well, ideally use Promises and async/await instead).
- Touche 10y ago
  Yes, but if the success handler throws you won't catch it. For example: promise.then(function(){ throw new Error('Oh no, not caught'); }, function(err){ }); But this will catch it: promise.then(function(){ throw new Error('Oh no!'); }).catch(function(err){ // Caught it ;) }); As will this: promise.then(function(){ throw new Error('Oh no!'); }).then(null, function(err){ // Caught it ;) }); Point being that your error handling need to be absolute last, and not paired with a success handler.
rhinoceraptor 10y ago
It's also an anti-pattern to not have an err as the first argument in an async callback.
- evv 10y ago
  Agreed! There is a handy package called `denodeify` which converts the callback pattern (with err) to promises. It works for every async function in node, except for `fs.exists` which inexplicably does not provide an error in the callback. Speaking of which, when is node going to actually start supporting promises natively? I've been using this denodeify hack for years!
  - franciscop 10y ago
    I would say there are dozens of projects that do this from a search I did a while back.
mikewhy 10y ago
Or ... return the promise in `fetchFirstUser`?
idbehold 10y ago
Mocha has support for returning promises in it() tests, so there's really no need to use callbacks.
- chukye 10y ago
  Well... mocha has support for callback since the beginning, callback is the simplest way to go, callback will never become obsolete, so there's really no need to use promises. :)
  - mikewhy 10y ago
    Promises are the backbone of async/await.
  - Akkuma 10y ago
    People should avoid using callbacks as they invert the chain of handling function calls. Rather than getting a result that you can work with, you move the logic into another function. Promises compose much more cleanly than callbacks do resulting in generally easier to follow code. If you call function x with a callback you either handle it immediately via your callback or you have to use some other paradigm to make it compose with other async code. You get no return value allowing you to write code that looks synchronous and forget about the beauty of async/await you've completed abandoned it with callbacks. If you use Promises you can do as you wish and combine them as you wish as they hold values and you can use them multiple times trivially with other Promises,
  - eyko 10y ago
    By the same reasoning, promises will never become obsolete because they're already supported. But if I were to bet on a hypothetical deprecation of one versus the other, I wouldn't bet on promises being deprecated, because they're part of the spec.
mattnewton 10y ago
Why was the solution to fallback to callbacks instead of just using promises all the way through? (Edit, removed mention of bluebird that would not have helped) The problem here isn't that they are being swallowed, it is that the tests have a constraint that the callback be called only once, which is conveniently the behavior of a then applied to the promise you would return.
- partisan 10y ago
  I agree. I don't think the headline of this article is correct. The author's expectations were incorrect and it certainly is a good thing for everyone to be aware of, but this is like saying "don't use pointers because I didn't release memory and it caused a memory leak". If you are using promises, understand how they work. The same for callbacks.
- jvranish42 10y ago
  I'm the author of that post. I could have certainly used promises all the way through, and in some cases that might be the better option. My example is contrived, but in the larger project that I was working in, almost all of the existing code used callbacks. Refactoring all that code to use promises, just to accommodate the two or three places that actually needed them, probably would not have been worth it.
  - vkjv 10y ago
    You don't need to refactor the existing code. You can change the calling code. // Promisifying a standard callback const myAsyncMethod = Bluebird.promisify(myCallbackMethod); // Or if you have weird callback passing and need // to manipulate the callback method being passed around function myAsyncMethod(...args) { return Bluebird.fromCallback(cb => myAsyncMethod(cb, ...args)); }
  - spankalee 10y ago
    It is worth it though because of the much better error handling in Promises and the ability to use async/await which makes error handling even more easy to get correct. I'd suggest making all functions that take callbacks also return Promises to make the transition easier: async function fetchFirstUser(iswhatIwant, callback) { const safeCallback = (user) => { try { callback(user); } catch (e) { console.error(e); } }; try { const user = await knex.first().from("users"); safeCallback(user); return user; } catch (e) { safeCallback(null); throw e; } } You can put this in a utility (and I'd recommend standardizing on Node's callback style): async doWithCallback(promise, callback) { const safeCallback = (error, result) => { if (callback == null) { return; } try { callback(null, result); } catch (e) { console.error(e); } }; try { const result = await promise; safeCallback(null, result); return result; } catch (e) { safeCallback(e); throw e; } } async function fetchFirstUser(iswhatIwant, callback) { return doWithCallback(knex.first().from("users"), callback); } At some point you can start warning when callbacks are passed in, then throwing, then remove the argument.
throwasehasdwi 10y ago
or just use async which fixes both of these arguably broken solutions for callback hell.
- hugozap 10y ago
  Most 'callback hell' situations are really just poor code structure. Easily solved with separated named functions.
  - throwasehasdwi 10y ago
    This is easy to say but hard to do in practice. Every JS application I've seen in production is littered with callback nightmares. It might be possible to avoid but in practice it's too difficult. async makes it easy and I don't see a downside to using it everywhere possible.
chatmasta 10y ago
Is there any typechecking library for JS that can warn of potentially "swallowed" promises? Does Flow do this?
- YCode 10y ago
  For debugging in Node I use: process.on("unhandledRejection", err => { console.error("Uncaught Promise Error: \n" + err.stack); console.error(err); });
  - mort96 10y ago
    That's better than letting it get swallowed, but in my experience is pretty terrible still, as it often just results in a stack trace where your code literally doesn't exist at all. In a somewhat big project, just having a vague error message without as much as a line number is hell.
- merkaloid 10y ago
  eslint has rules to make you return a value on every promise, whilst Node itself will complain about unhandled promise rejection
deleted 10y ago
[deleted]
hugozap 10y ago
Callbacks are simpler, easier to understand and explain. 'callback hell' is usually the result of 'poor code structure' that get's fixed with separated named functions.
- davexunit 10y ago
  Writing in continuation passing style manually isn't great. It really does become confusing and hard to debug very quickly. That said, promises also aren't the solution, because you still have to contort your code to be compatible with promises. Further more, async/await is yet another non-solution because now you have to differentiate between synchronous and asynchronous functions. Still, both of these are improvements upon simple callbacks. What JavaScript is lacking is a real implementation of "communicating sequential processes" a la Go, ConcurrentML, etc.
uranian 10y ago
Better never use Promises at all, it is no solution for a problem, instead it makes code only more ambiguous IMHO. And indeed mixing it with callbacks or async libraries makes it even worse.. I've once had a very nasty bug keeping me busy for days, where somewhere in a complex structure of multiple promises I somehow forgot to return a call to a resolve. With all these new Promise(), .then(), .catch(), resolve's and reject's nested it's very easy to miss one, and very hard to read where the bug sits. Maybe it's just me, but I never had such a severe bug with the simplicity of the standard async callback pattern. I Never used promises again, still enjoying that decision.
- l1ambda 10y ago
  Promises enable async "stacktraces" though. With just callbacks, when an exception is thrown there is not the complete "stack", and little hope of debugging.
- josteink 10y ago
  > I've once had a very nasty bug keeping me busy for days, where somewhere in a complex structure of multiple promises I somehow forgot to return a call to a resolve. Promises allow, enable and encourage composition. If you use them correctly no single promise should ever be so large that this becomes a problem. That said, you can write spaghetti in any language and platform. If you're the kind to write 100+ line async functions, no framework or technique will help you keep your code tight and solid.
- wging 10y ago
  My experience has been exactly the reverse. Promises have made async code much easier to read and understand. There is a little learning curve, but it's not so bad.
  - kevinmannix 10y ago
    Totally agree. There's a sense of satisfaction when refactoring legacy callback code to a simple promise chain. It's even better when the functions called in succession have names that read out concisely and clearly the behavior of the Promise chain as a whole
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 ago
  If 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 ago
    I 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.
franciscop 10y ago
I just wrote a reply on Medium to this article: https://medium.com/@fpresencia/please-mix-promises-and-callbacks-b2d15da6166c https://medium.com/@fpresencia/please-mix-promises-and-callb... Takeaways: - It has the wrong signature! The callback is the second parameter but is being passed as the first, so it will never be called. - Even if that's a typo, don't catch what you intend to throw as an exception. Use the callback as the second parameter of the `then` and let it fail. The error will stil l be swallowed, but in a more predictable way. - Better yet, use async/await if possible! It makes promises a joy to work with.
- jaquers 10y ago
  "It has the wrong signature!" - This x1000.
dmix 10y ago
> Knex queries return Bluebird promises by default I haven't kept up with JS trends recently. Are these promise wrapper libraries like Bluebird still relevant now that ES6 is out?
- vkjv 10y ago
  Bluebird is more than just a promise implementation. It is also a general purpose promise utility library. E.g., providing `promisify` wrappers for callback driven code and functional collection methods like `map` / `filter` / `reduce`.
erikpukinskis 10y ago
This is another great reason never to use promises. "Callback Hell" is FUD spread by people who somehow missed the memo about the fact that you can add functions to your code. http://callbackhell.com http://callbackhell.com explains it quite well. In running away from "Callback Hell" the JavaScript community ran straight into "Control Structure Hell" which is far, far worse.
jaquers 10y ago
There is a point nobody here is making, which is that all the examples use the wrong signature for callbacks, which is function(err, result). The problem is not interop between the two, which is fairly easy - the problem arises when not using standard signatures. Seems like all the fuss could be avoided too because Mocha supports both promises and callbacks. function asCallback(promise, callback) { return Promise.resolve(promise) .then(result => callback(null, result)) .catch(callback) } // returns a new fn that takes same args which returns promise instead function asPromise(fn) { return (...args) => new Promise((resolve, reject) => { fn(...args, (err, result) => { if (err) return reject(err); resolve(result) }) }) }
qubyte 10y ago
Weird things can happen with event emitters mixed up with promises too. I wrote a short piece with an example: https://qubyte.codes/blog/promises-and-nodejs-event-emitters-dont-mix https://qubyte.codes/blog/promises-and-nodejs-event-emitters... (The title is a bit overstated, since it's really about the specific example of unhandled error emissions in a promise chain.) Regardless of the merits or problems of either post, a lesson is to be careful around promises when errors are involved.

65 points by philk10 10y ago

Wouldn't using the second parameter of .then() to catch exceptions work instead of .catch()? http://stackoverflow.com/a/33278420 http://stackoverflow.com/a/33278420

pornel 10y ago
It doesn't matter. The problem is that `callback(null)` throws, and in the `Promise` land this will never result in an uncaught exception. An exception directly inside any `Promise` callback produces another `Promise` in the chain instead. You could at best use `setTimeout` or `nextTick` to escape from `Promise`'s call stack and throw there.
- mariusc23 10y ago
  Good point! I thought of solving the callback getting called twice problem and didn't think of the bigger picture that mocha needs to catch the assertion error. I like the setTimeout/nextTick trick (well, ideally use Promises and async/await instead).
Touche 10y ago
Yes, but if the success handler throws you won't catch it. For example: promise.then(function(){ throw new Error('Oh no, not caught'); }, function(err){ }); But this will catch it: promise.then(function(){ throw new Error('Oh no!'); }).catch(function(err){ // Caught it ;) }); As will this: promise.then(function(){ throw new Error('Oh no!'); }).then(null, function(err){ // Caught it ;) }); Point being that your error handling need to be absolute last, and not paired with a success handler.

rhinoceraptor 10y ago

It's also an anti-pattern to not have an err as the first argument in an async callback.

evv 10y ago
Agreed! There is a handy package called `denodeify` which converts the callback pattern (with err) to promises. It works for every async function in node, except for `fs.exists` which inexplicably does not provide an error in the callback. Speaking of which, when is node going to actually start supporting promises natively? I've been using this denodeify hack for years!
- franciscop 10y ago
  I would say there are dozens of projects that do this from a search I did a while back.

mikewhy 10y ago

Or ... return the promise in `fetchFirstUser`?

idbehold 10y ago

Mocha has support for returning promises in it() tests, so there's really no need to use callbacks.

chukye 10y ago
Well... mocha has support for callback since the beginning, callback is the simplest way to go, callback will never become obsolete, so there's really no need to use promises. :)
- mikewhy 10y ago
  Promises are the backbone of async/await.
- Akkuma 10y ago
  People should avoid using callbacks as they invert the chain of handling function calls. Rather than getting a result that you can work with, you move the logic into another function. Promises compose much more cleanly than callbacks do resulting in generally easier to follow code. If you call function x with a callback you either handle it immediately via your callback or you have to use some other paradigm to make it compose with other async code. You get no return value allowing you to write code that looks synchronous and forget about the beauty of async/await you've completed abandoned it with callbacks. If you use Promises you can do as you wish and combine them as you wish as they hold values and you can use them multiple times trivially with other Promises,
- eyko 10y ago
  By the same reasoning, promises will never become obsolete because they're already supported. But if I were to bet on a hypothetical deprecation of one versus the other, I wouldn't bet on promises being deprecated, because they're part of the spec.

mattnewton 10y ago

Why was the solution to fallback to callbacks instead of just using promises all the way through? (Edit, removed mention of bluebird that would not have helped) The problem here isn't that they are being swallowed, it is that the tests have a constraint that the callback be called only once, which is conveniently the behavior of a then applied to the promise you would return.

partisan 10y ago
I agree. I don't think the headline of this article is correct. The author's expectations were incorrect and it certainly is a good thing for everyone to be aware of, but this is like saying "don't use pointers because I didn't release memory and it caused a memory leak". If you are using promises, understand how they work. The same for callbacks.
jvranish42 10y ago
I'm the author of that post. I could have certainly used promises all the way through, and in some cases that might be the better option. My example is contrived, but in the larger project that I was working in, almost all of the existing code used callbacks. Refactoring all that code to use promises, just to accommodate the two or three places that actually needed them, probably would not have been worth it.
- vkjv 10y ago
  You don't need to refactor the existing code. You can change the calling code. // Promisifying a standard callback const myAsyncMethod = Bluebird.promisify(myCallbackMethod); // Or if you have weird callback passing and need // to manipulate the callback method being passed around function myAsyncMethod(...args) { return Bluebird.fromCallback(cb => myAsyncMethod(cb, ...args)); }
- spankalee 10y ago
  It is worth it though because of the much better error handling in Promises and the ability to use async/await which makes error handling even more easy to get correct. I'd suggest making all functions that take callbacks also return Promises to make the transition easier: async function fetchFirstUser(iswhatIwant, callback) { const safeCallback = (user) => { try { callback(user); } catch (e) { console.error(e); } }; try { const user = await knex.first().from("users"); safeCallback(user); return user; } catch (e) { safeCallback(null); throw e; } } You can put this in a utility (and I'd recommend standardizing on Node's callback style): async doWithCallback(promise, callback) { const safeCallback = (error, result) => { if (callback == null) { return; } try { callback(null, result); } catch (e) { console.error(e); } }; try { const result = await promise; safeCallback(null, result); return result; } catch (e) { safeCallback(e); throw e; } } async function fetchFirstUser(iswhatIwant, callback) { return doWithCallback(knex.first().from("users"), callback); } At some point you can start warning when callbacks are passed in, then throwing, then remove the argument.

throwasehasdwi 10y ago

or just use async which fixes both of these arguably broken solutions for callback hell.

hugozap 10y ago
Most 'callback hell' situations are really just poor code structure. Easily solved with separated named functions.
- throwasehasdwi 10y ago
  This is easy to say but hard to do in practice. Every JS application I've seen in production is littered with callback nightmares. It might be possible to avoid but in practice it's too difficult. async makes it easy and I don't see a downside to using it everywhere possible.

chatmasta 10y ago

Is there any typechecking library for JS that can warn of potentially "swallowed" promises? Does Flow do this?

YCode 10y ago
For debugging in Node I use: process.on("unhandledRejection", err => { console.error("Uncaught Promise Error: \n" + err.stack); console.error(err); });
- mort96 10y ago
  That's better than letting it get swallowed, but in my experience is pretty terrible still, as it often just results in a stack trace where your code literally doesn't exist at all. In a somewhat big project, just having a vague error message without as much as a line number is hell.
merkaloid 10y ago
eslint has rules to make you return a value on every promise, whilst Node itself will complain about unhandled promise rejection

deleted 10y ago

[deleted]

hugozap 10y ago

Callbacks are simpler, easier to understand and explain. 'callback hell' is usually the result of 'poor code structure' that get's fixed with separated named functions.

davexunit 10y ago
Writing in continuation passing style manually isn't great. It really does become confusing and hard to debug very quickly. That said, promises also aren't the solution, because you still have to contort your code to be compatible with promises. Further more, async/await is yet another non-solution because now you have to differentiate between synchronous and asynchronous functions. Still, both of these are improvements upon simple callbacks. What JavaScript is lacking is a real implementation of "communicating sequential processes" a la Go, ConcurrentML, etc.

uranian 10y ago

Better never use Promises at all, it is no solution for a problem, instead it makes code only more ambiguous IMHO. And indeed mixing it with callbacks or async libraries makes it even worse.. I've once had a very nasty bug keeping me busy for days, where somewhere in a complex structure of multiple promises I somehow forgot to return a call to a resolve. With all these new Promise(), .then(), .catch(), resolve's and reject's nested it's very easy to miss one, and very hard to read where the bug sits. Maybe it's just me, but I never had such a severe bug with the simplicity of the standard async callback pattern. I Never used promises again, still enjoying that decision.

l1ambda 10y ago
Promises enable async "stacktraces" though. With just callbacks, when an exception is thrown there is not the complete "stack", and little hope of debugging.
josteink 10y ago
> I've once had a very nasty bug keeping me busy for days, where somewhere in a complex structure of multiple promises I somehow forgot to return a call to a resolve. Promises allow, enable and encourage composition. If you use them correctly no single promise should ever be so large that this becomes a problem. That said, you can write spaghetti in any language and platform. If you're the kind to write 100+ line async functions, no framework or technique will help you keep your code tight and solid.
wging 10y ago
My experience has been exactly the reverse. Promises have made async code much easier to read and understand. There is a little learning curve, but it's not so bad.
- kevinmannix 10y ago
  Totally agree. There's a sense of satisfaction when refactoring legacy callback code to a simple promise chain. It's even better when the functions called in succession have names that read out concisely and clearly the behavior of the Promise chain as a whole

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 ago
If 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 ago
  I 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.

franciscop 10y ago

I just wrote a reply on Medium to this article: https://medium.com/@fpresencia/please-mix-promises-and-callbacks-b2d15da6166c https://medium.com/@fpresencia/please-mix-promises-and-callb... Takeaways: - It has the wrong signature! The callback is the second parameter but is being passed as the first, so it will never be called. - Even if that's a typo, don't catch what you intend to throw as an exception. Use the callback as the second parameter of the `then` and let it fail. The error will stil l be swallowed, but in a more predictable way. - Better yet, use async/await if possible! It makes promises a joy to work with.

jaquers 10y ago
"It has the wrong signature!" - This x1000.

dmix 10y ago

> Knex queries return Bluebird promises by default I haven't kept up with JS trends recently. Are these promise wrapper libraries like Bluebird still relevant now that ES6 is out?

vkjv 10y ago
Bluebird is more than just a promise implementation. It is also a general purpose promise utility library. E.g., providing `promisify` wrappers for callback driven code and functional collection methods like `map` / `filter` / `reduce`.

erikpukinskis 10y ago

This is another great reason never to use promises. "Callback Hell" is FUD spread by people who somehow missed the memo about the fact that you can add functions to your code. http://callbackhell.com http://callbackhell.com explains it quite well. In running away from "Callback Hell" the JavaScript community ran straight into "Control Structure Hell" which is far, far worse.

jaquers 10y ago

There is a point nobody here is making, which is that all the examples use the wrong signature for callbacks, which is function(err, result). The problem is not interop between the two, which is fairly easy - the problem arises when not using standard signatures. Seems like all the fuss could be avoided too because Mocha supports both promises and callbacks. function asCallback(promise, callback) { return Promise.resolve(promise) .then(result => callback(null, result)) .catch(callback) } // returns a new fn that takes same args which returns promise instead function asPromise(fn) { return (...args) => new Promise((resolve, reject) => { fn(...args, (err, result) => { if (err) return reject(err); resolve(result) }) }) }

qubyte 10y ago

Weird things can happen with event emitters mixed up with promises too. I wrote a short piece with an example: https://qubyte.codes/blog/promises-and-nodejs-event-emitters-dont-mix https://qubyte.codes/blog/promises-and-nodejs-event-emitters... (The title is a bit overstated, since it's really about the specific example of unhandled error emissions in a promise chain.) Regardless of the merits or problems of either post, a lesson is to be careful around promises when errors are involved.