no-callback-in-promise: call should be allowed if promise does not return

[gabmontes]

Description

As a general rule, callbacks should not be called inside promises and, as a general rule, it is advisable to "nodeify" the promise call to properly work within a callback context.

Having said that, there are some exceptions where it is absolutely reasonable to call callbacks within promises, i.e. when a promise call inside an async handler does not return. Given there is no promise returned, the only safe way to get out of the handler is to call the callback.

Steps to Reproduce

Given i.e. express or restify middleware functions shall call next and don't handle promises (yet?), a middleware shall be written as:

function middleware (req, res, next) {
  doAsyncOp(req, res)
    .then(() => next())
             // ^^^ "Avoid calling back inside of a promise" is shown
    .catch(next)
}

In the case the framework supports promises, as mocha, calling done instead of returning a promise should be a flag:

it('should not be done like this', function (done) {
  return doAsyncTestOp()
    .then(() => done())
    .catch(done)
}

it('should use this pattern instead', function () {
  return doAsyncTestOp()
}

So, the rule should be updated to trigger only when a callback is called and a promise is returned because is semantically incorrect to do both at the same time but it is allowed to do otherwise.

Expected behavior: The rule is triggered even when no promise is returned.

Actual behavior: The rule should only trigger if a callback is called and a promise is returned.

Versions

• ESLint version: 4.19.0
• eslint-plugin-promise version: 3.7.0

[marcogrcr]

Hmm, but wouldn't that result in the unintended behavior of potentially invoking the callback twice?

index.js

doAsyncOp()
  .then(() => callback())
  .catch(callback);

function callback() {
  console.log("Callback got called");
  throw new Error();
}

Output:

$ node .
Callback got called
Callback got called

This is why nodeify uses setImmediate/process.nextTick/setTimeout to wrap the callback invocation:

doAsyncOp()
  .then(() => setImmediate(callback))
  .catch(() => setImmediate(callback));

[bwertman-rc]

This is an issue with the example, not the premise - you could write it like this and it wouldn't get called twice, but the lint issue is still present:

doAsyncOp()
  .then(() => callback(), callback);

function callback() {
  console.log("Callback got called");
  throw new Error();
}