feat(catch-or-return): add allowThenStrict option (#522)

eslint-community · Oct 16, 2024 · 53be970 · 53be970
1 parent bbd048b
commit 53be970
Show file tree

Hide file tree

Showing 3 changed files with 167 additions and 11 deletions.
diff --git a/__tests__/catch-or-return.js b/__tests__/catch-or-return.js
@@ -53,16 +53,10 @@ ruleTester.run('catch-or-return', rule, {
       options: [{ allowThen: true }],
     },
 
-    // allowThen - .then(null, fn)
-    { code: 'frank().then(a, b)', options: [{ allowThen: true }] },
     {
       code: 'frank().then(a).then(b).then(null, c)',
       options: [{ allowThen: true }],
     },
-    {
-      code: 'frank().then(a).then(b).then(c, d)',
-      options: [{ allowThen: true }],
-    },
     {
       code: 'frank().then(a).then(b).then().then().then(null, doIt)',
       options: [{ allowThen: true }],
@@ -73,10 +67,15 @@ ruleTester.run('catch-or-return', rule, {
     },
 
     // allowThen - .then(fn, fn)
+    { code: 'frank().then(a, b)', options: [{ allowThen: true }] },
     {
       code: 'frank().then(go).then(zam, doIt)',
       options: [{ allowThen: true }],
     },
+    {
+      code: 'frank().then(a).then(b).then(c, d)',
+      options: [{ allowThen: true }],
+    },
     {
       code: 'frank().then(go).then().then().then().then(wham, doIt)',
       options: [{ allowThen: true }],
@@ -90,6 +89,37 @@ ruleTester.run('catch-or-return', rule, {
       options: [{ allowThen: true }],
     },
 
+    // allowThenStrict - .then(null, fn)
+    {
+      code: 'frank().then(go).then(null, doIt)',
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(go).then().then().then().then(null, doIt)',
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(go).then().then(null, function() { /* why bother */ })',
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank.then(go).then(to).then(null, jail)',
+      options: [{ allowThenStrict: true }],
+    },
+
+    {
+      code: 'frank().then(a).then(b).then(null, c)',
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(a).then(b).then().then().then(null, doIt)',
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(a).then(b).then(null, function() { /* why bother */ })',
+      options: [{ allowThenStrict: true }],
+    },
+
     // allowFinally - .finally(fn)
     {
       code: 'frank().then(go).catch(doIt).finally(fn)',
@@ -234,5 +264,76 @@ ruleTester.run('catch-or-return', rule, {
       code: 'frank().catch(go).someOtherMethod()',
       errors: [{ message: catchMessage }],
     },
+
+    // .then(null, fn)
+    {
+      code: 'frank().then(a).then(b).then(null, c)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(a).then(b).then().then().then(null, doIt)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(a).then(b).then(null, function() { /* why bother */ })',
+      errors: [{ message: catchMessage }],
+    },
+
+    // .then(fn, fn)
+    {
+      code: 'frank().then(a, b)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(go).then(zam, doIt)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(a).then(b).then(c, d)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(go).then().then().then().then(wham, doIt)',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank().then(go).then().then(function() {}, function() { /* why bother */ })',
+      errors: [{ message: catchMessage }],
+    },
+    {
+      code: 'frank.then(go).then(to).then(pewPew, jail)',
+      errors: [{ message: catchMessage }],
+    },
+
+    {
+      code: 'frank().then(a, b)',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(go).then(zam, doIt)',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(a).then(b).then(c, d)',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(go).then().then().then().then(wham, doIt)',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank().then(go).then().then(function() {}, function() { /* why bother */ })',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
+    {
+      code: 'frank.then(go).then(to).then(pewPew, jail)',
+      errors: [{ message: catchMessage }],
+      options: [{ allowThenStrict: true }],
+    },
   ],
 })
diff --git a/docs/rules/catch-or-return.md b/docs/rules/catch-or-return.md
@@ -32,9 +32,56 @@ function doSomethingElse() {
 
 ##### `allowThen`
 
-You can pass an `{ allowThen: true }` as an option to this rule to allow for
-`.then(null, fn)` to be used instead of `catch()` at the end of the promise
-chain.
+The second argument to `then()` can also be used to handle a promise rejection,
+but it won't catch any errors from the first argument callback. Because of this,
+this rule reports usage of `then()` with two arguments without `catch()` by
+default.
+
+However, you can use `{ allowThen: true }` to allow using `then()` with two
+arguments instead of `catch()` to handle promise rejections.
+
+Examples of **incorrect** code for the default `{ allowThen: false }` option:
+
+```js
+myPromise.then(doSomething, handleErrors)
+myPromise.then(null, handleErrors)
+```
+
+Examples of **correct** code for the `{ allowThen: true }` option:
+
+```js
+myPromise.then(doSomething, handleErrors)
+myPromise.then(null, handleErrors)
+myPromise.then(doSomething).catch(handleErrors)
+```
+
+##### `allowThenStrict`
+
+`allowThenStrict` is similar to `allowThen` but it only permits `then` when the
+fulfillment handler is `null`. This option ensures that the final rejected
+handler works like a `catch` and can handle any uncaught errors from the final
+`then`.
+
+Examples of **incorrect** code for the default `{ allowThenStrict: false }`
+option:
+
+```js
+myPromise.then(doSomething, handleErrors)
+myPromise.then(null, handleErrors)
+```
+
+Examples of **correct** code for the `{ allowThenStrict: true }` option:
+
+```js
+myPromise.then(null, handleErrors)
+myPromise.then(doSomething).catch(handleErrors)
+```
+
+Examples of **incorrect** code for the `{ allowThenStrict: true }` option:
+
+```js
+myPromise.then(doSomething, handleErrors)
+```
 
 ##### `allowFinally`
 

diff --git a/rules/catch-or-return.js b/rules/catch-or-return.js
@@ -30,6 +30,9 @@ module.exports = {
           allowThen: {
             type: 'boolean',
           },
+          allowThenStrict: {
+            type: 'boolean',
+          },
           terminationMethod: {
             oneOf: [
               { type: 'string' },
@@ -49,6 +52,7 @@ module.exports = {
   create(context) {
     const options = context.options[0] || {}
     const allowThen = options.allowThen
+    const allowThenStrict = options.allowThenStrict
     const allowFinally = options.allowFinally
     let terminationMethod = options.terminationMethod || 'catch'
 
@@ -59,11 +63,15 @@ module.exports = {
     function isAllowedPromiseTermination(expression) {
       // somePromise.then(a, b)
       if (
-        allowThen &&
+        (allowThen || allowThenStrict) &&
         expression.type === 'CallExpression' &&
         expression.callee.type === 'MemberExpression' &&
         expression.callee.property.name === 'then' &&
-        expression.arguments.length === 2
+        expression.arguments.length === 2 &&
+        // somePromise.then(null, b)
+        ((allowThen && !allowThenStrict) ||
+          (expression.arguments[0].type === 'Literal' &&
+            expression.arguments[0].value === null))
       ) {
         return true
       }