From 0a4d2fadf01dacc8760e7b7ca22ad691abe8fa31 Mon Sep 17 00:00:00 2001 From: Brett Zamir Date: Tue, 30 Jul 2024 11:46:43 +0800 Subject: [PATCH 1/3] docs(`catch-or-return`): improve docs re: `allowThen` option --- docs/rules/catch-or-return.md | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/rules/catch-or-return.md b/docs/rules/catch-or-return.md index 1833ca57..668ce4d7 100644 --- a/docs/rules/catch-or-return.md +++ b/docs/rules/catch-or-return.md @@ -32,9 +32,28 @@ 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) +``` ##### `allowFinally` From 3d22840dbfe4b6f19a4980cc2b575cd649536a41 Mon Sep 17 00:00:00 2001 From: Brett Zamir Date: Tue, 30 Jul 2024 11:47:21 +0800 Subject: [PATCH 2/3] test: correctly reorder tests and check default `allowThen: false` for `catch-or-return` --- __tests__/catch-or-return.js | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/__tests__/catch-or-return.js b/__tests__/catch-or-return.js index 324f1482..3d289435 100644 --- 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 }], @@ -234,5 +233,45 @@ 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 }], + }, ], }) From 1a0161f0e29c1b99ef9e2de1b0129225d132997a Mon Sep 17 00:00:00 2001 From: Brett Zamir Date: Tue, 30 Jul 2024 12:01:52 +0800 Subject: [PATCH 3/3] feat: add `allowThenStrict` option; fixes #52 --- __tests__/catch-or-return.js | 62 +++++++++++++++++++++++++++++++++++ docs/rules/catch-or-return.md | 28 ++++++++++++++++ rules/catch-or-return.js | 12 +++++-- 3 files changed, 100 insertions(+), 2 deletions(-) diff --git a/__tests__/catch-or-return.js b/__tests__/catch-or-return.js index 3d289435..03b1fa3e 100644 --- a/__tests__/catch-or-return.js +++ b/__tests__/catch-or-return.js @@ -89,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)', @@ -273,5 +304,36 @@ ruleTester.run('catch-or-return', rule, { 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 index 668ce4d7..2077d046 100644 --- a/docs/rules/catch-or-return.md +++ b/docs/rules/catch-or-return.md @@ -55,6 +55,34 @@ 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` You can pass an `{ allowFinally: true }` as an option to this rule to allow for diff --git a/rules/catch-or-return.js b/rules/catch-or-return.js index c12b82fc..1baae113 100644 --- 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 }