feat(no-callback-in-promise): add timeoutsErr option; fixes #167

Also: - fix(`no-callback-in-promise`): ensure timeouts do not err (by default); fixes #220
eslint-community · Jul 30, 2024 · cee71eb · cee71eb
1 parent 94c9834
commit cee71eb
Show file tree

Hide file tree

Showing 3 changed files with 145 additions and 21 deletions.
diff --git 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
@@ -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
@@ -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',