diff --git a/__tests__/no-callback-in-promise.js b/__tests__/no-callback-in-promise.js index eb59c9f8..7a7a27f9 100644 --- a/__tests__/no-callback-in-promise.js +++ b/__tests__/no-callback-in-promise.js @@ -17,13 +17,13 @@ ruleTester.run('no-callback-in-promise', rule, { 'function thing(callback) { callback() }', 'doSomething(function(err) { callback(err) })', - // TODO: support safe callbacks (see #220) - // 'whatever.then((err) => { process.nextTick(() => cb()) })', - // 'whatever.then((err) => { setImmediate(() => cb())) })', - // 'whatever.then((err) => setImmediate(() => cb()))', - // 'whatever.then((err) => process.nextTick(() => cb()))', - // 'whatever.then((err) => process.nextTick(cb))', - // 'whatever.then((err) => setImmediate(cb))', + // Support safe callbacks (#220) + 'whatever.then((err) => { process.nextTick(() => cb()) })', + 'whatever.then((err) => { setImmediate(() => cb()) })', + 'whatever.then((err) => setImmediate(() => cb()))', + 'whatever.then((err) => process.nextTick(() => cb()))', + 'whatever.then((err) => process.nextTick(cb))', + 'whatever.then((err) => setImmediate(cb))', // arrow functions and other things 'let thing = (cb) => cb()', @@ -97,5 +97,94 @@ ruleTester.run('no-callback-in-promise', rule, { code: 'a.catch(function(err) { callback(err) })', errors: [{ message: errorMessage }], }, + + // #167 + { + code: ` + function wait (callback) { + return Promise.resolve() + .then(() => { + setTimeout(callback); + }); + } + `, + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: ` + function wait (callback) { + return Promise.resolve() + .then(() => { + setTimeout(() => callback()); + }); + } + `, + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + + { + code: 'whatever.then((err) => { process.nextTick(() => cb()) })', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: 'whatever.then((err) => { setImmediate(() => cb()) })', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: 'whatever.then((err) => setImmediate(() => cb()))', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: 'whatever.then((err) => process.nextTick(() => cb()))', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: 'whatever.then((err) => process.nextTick(cb))', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, + { + code: 'whatever.then((err) => setImmediate(cb))', + errors: [{ message: errorMessage }], + options: [ + { + timeoutsErr: true, + }, + ], + }, ], }) diff --git a/docs/rules/no-callback-in-promise.md b/docs/rules/no-callback-in-promise.md index e79618d7..f77ec4d6 100644 --- a/docs/rules/no-callback-in-promise.md +++ b/docs/rules/no-callback-in-promise.md @@ -31,7 +31,14 @@ Callback got called with: null data Callback got called with: My error null ``` -**How to fix it?** +## Options + +### `timeoutsErr` + +Boolean as to whether callbacks in timeout functions like `setTimeout` will err. +Defaults to `false`. + +## How to fix it? Ensure that your callback invocations are wrapped by a deferred execution function such as: diff --git a/rules/no-callback-in-promise.js b/rules/no-callback-in-promise.js index bc786749..7bcf5cbd 100644 --- a/rules/no-callback-in-promise.js +++ b/rules/no-callback-in-promise.js @@ -7,11 +7,28 @@ const { getAncestors } = require('./lib/eslint-compat') const getDocsUrl = require('./lib/get-docs-url') -const hasPromiseCallback = require('./lib/has-promise-callback') const isInsidePromise = require('./lib/is-inside-promise') const isCallback = require('./lib/is-callback') const CB_BLACKLIST = ['callback', 'cb', 'next', 'done'] +const TIMEOUT_WHITELIST = [ + 'setImmediate', + 'setTimeout', + 'requestAnimationFrame', + 'nextTick', +] + +const isInsideTimeout = (node) => { + const isFunctionExpression = + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + const parent = node.parent || {} + const callee = parent.callee || {} + const name = (callee.property && callee.property.name) || callee.name || '' + const parentIsTimeout = TIMEOUT_WHITELIST.includes(name) + const isInCB = isFunctionExpression && parentIsTimeout + return isInCB +} module.exports = { meta: { @@ -34,32 +51,43 @@ module.exports = { type: 'string', }, }, + timeoutsErr: { + type: 'boolean', + }, }, additionalProperties: false, }, ], }, create(context) { + const { timeoutsErr = false } = context.options[0] || {} + return { CallExpression(node) { const options = context.options[0] || {} const exceptions = options.exceptions || [] if (!isCallback(node, exceptions)) { - // in general we send you packing if you're not a callback - // but we also need to watch out for whatever.then(cb) - if (hasPromiseCallback(node)) { - const name = - node.arguments && node.arguments[0] && node.arguments[0].name - if (!exceptions.includes(name) && CB_BLACKLIST.includes(name)) { - context.report({ - node: node.arguments[0], - messageId: 'callback', - }) - } + const callingName = node.callee.name || node.callee.property?.name + const name = + node.arguments && node.arguments[0] && node.arguments[0].name + if ( + !exceptions.includes(name) && + CB_BLACKLIST.includes(name) && + (timeoutsErr || !TIMEOUT_WHITELIST.includes(callingName)) + ) { + context.report({ + node: node.arguments[0], + messageId: 'callback', + }) } return } - if (getAncestors(context, node).some(isInsidePromise)) { + + const ancestors = getAncestors(context, node) + if ( + ancestors.some(isInsidePromise) && + (timeoutsErr || !ancestors.some(isInsideTimeout)) + ) { context.report({ node, messageId: 'callback',