Never Mix Promises and Callbacks in NodeJS | Hacker News Reader

Never Mix Promises and Callbacks in NodeJS

spin.atomicobject.com

65 pointsphilk1044 comments

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.

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.

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.

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

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.

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.

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.

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

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.

I just wrote a reply on Medium to this article: 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.

"It has the wrong signature!" - This x1000.

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

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!

I would say there are dozens of projects that do this from a search I did a while back.

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

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

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

Promises are the backbone of async/await.

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,

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.

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

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

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

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.

Wouldn't using the second parameter of .then() to catch exceptions work instead of .catch()?

http://stackoverflow.com/a/33278420

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.

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

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.

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

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

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.

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.

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

For debugging in Node I use:

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

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.

eslint has rules to make you return a value on every promise, whilst Node itself will complain about unhandled promise rejection

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.

Promises enable async "stacktraces" though. With just callbacks, when an exception is thrown there is not the complete "stack", and little hope of debugging.

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

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.

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

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.

> 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?

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

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 explains it quite well.

In running away from "Callback Hell" the JavaScript community ran straight into "Control Structure Hell" which is far, far worse.

Reply on news.ycombinator.com