If `completer` rejects, callback is called twice

[jwalton]

I was just browsing through the code and came across this:

    result.catch(cb).then(function (result) {
      process.nextTick(function () { cb(null, result) })
    }

So, if result rejects, we'll .catch(cb) and call the callback, but then .then(function(result) {... will be called with whatever cb returns, so we'll call cb again next tick. This should be:

    result.then(
      function (result) {
        process.nextTick(function () { cb(null, result) })
      },
      cb
    );

(Edit to add link to code)

[LinusU]

Nice catch!

It should probably be like this as to not swallow any errors thrown from the callback in the error path:

    result.then(
      function (result) {
        process.nextTick(function () { cb(null, result) })
      },
      function (err) {
        process.nextTick(function () { cb(err) })
      }
    );

[jwalton]

Well, that means any exception from the callback will be turned into an uncaught exception. If instead you just do:

    result.then(
      function (result) {cb(null, result);},
      cb
    );

then the error gets propagated like any other error. Of course, the callback here is node.js code, so hopefully it's not going to throw random stuff at us.

If it were me, I'd do:

var pb = require('promise-breaker');

function wrapCompleter (completer) {
  return function(line, cb) {pb.apply(completer, null, [line], cb;};
}

Which covers the case where completer takes a callback, or returns a Promise, or even returns a scalar. But that's plugging my own library. ;)

[LinusU]

Well, that means any exception from the callback will be turned into an uncaught exception.

Yes, which I think is very important, since that is what happens when using other callback based apis.

then the error gets propagated like any other error.

But then it will be turned into an unhandled rejection instead?

[jwalton]

Oh, you're right. :)