
Never Mix Promises and Callbacks in NodeJS - philk10
https://spin.atomicobject.com/2017/04/06/nodejs-promises-callbacks/#.WOeUlCGS_0c.hackernews
======
spankalee
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_
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.

~~~
spankalee
I 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));
        }

------
mattnewton
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.

~~~
jvranish42
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.

~~~
spankalee
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.

------
franciscop
I just wrote a reply on Medium to this article:
[https://medium.com/@fpresencia/please-mix-promises-and-
callb...](https://medium.com/@fpresencia/please-mix-promises-and-
callbacks-b2d15da6166c)

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
"It has the wrong signature!" \- This x1000.

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

~~~
evv
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
I would say there are _dozens_ of projects that do this from a search I did a
while back.

------
jaquers
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) 
          })
        })
      }

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

~~~
chukye
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. :)

~~~
Akkuma
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,

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

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

~~~
hugozap
Most 'callback hell' situations are really just poor code structure. Easily
solved with separated named functions.

~~~
throwasehasdwi
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.

------
mariusc23
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
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
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).

------
qubyte
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...](https://qubyte.codes/blog/promises-and-nodejs-event-emitters-
dont-mix)

(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.

------
hugozap
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
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.

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

~~~
YCode
For debugging in Node I use:

    
    
        process.on("unhandledRejection", err => {
            console.error("Uncaught Promise Error: \n" + err.stack);
            console.error(err);
        });

~~~
mort96
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.

------
uranian
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.

~~~
wging
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
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

~~~
uranian
haha, totally happy for you guys enjoying Promises, it apparently works for
you and that's what counts.

I also enjoy refactoring callback code, only without the use of Promises,
because I don't prefer them to compose and create a nice readable flow. There
is a little learning curve though, but it's not so bad.

------
dmix
> 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
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
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.

