Skip to content

Commit

Permalink
Add no-single-promise-in-promise-methods rule (#2258)
Browse files Browse the repository at this point in the history
Co-authored-by: fisker <[email protected]>
Co-authored-by: Sindre Sorhus <[email protected]>
  • Loading branch information
3 people authored Feb 23, 2024
1 parent 1792d33 commit 8f0ee89
Show file tree
Hide file tree
Showing 9 changed files with 1,473 additions and 0 deletions.
1 change: 1 addition & 0 deletions configs/recommended.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ module.exports = {
'unicorn/no-null': 'error',
'unicorn/no-object-as-default-parameter': 'error',
'unicorn/no-process-exit': 'error',
'unicorn/no-single-promise-in-promise-methods': 'error',
'unicorn/no-static-only-class': 'error',
'unicorn/no-thenable': 'error',
'unicorn/no-this-assignment': 'error',
Expand Down
50 changes: 50 additions & 0 deletions docs/rules/no-single-promise-in-promise-methods.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Disallow passing single-element arrays to `Promise` methods

💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs).

🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

Passing a single-element array to `Promise.all()`, `Promise.any()`, or `Promise.race()` is likely a mistake.

## Fail

```js
const foo = await Promise.all([promise]);
```

```js
const foo = await Promise.any([promise]);
```

```js
const foo = await Promise.race([promise]);
```

```js
const promise = Promise.all([nonPromise]);
```

## Pass

```js
const foo = await promise;
```

```js
const promise = Promise.resolve(nonPromise);
```

```js
const foo = await Promise.all(promises);
```

```js
const foo = await Promise.any([promise, anotherPromise]);
```

```js
const [{value: foo, reason: error}] = await Promise.allSettled([promise]);
```
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
| [no-null](docs/rules/no-null.md) | Disallow the use of the `null` literal. || 🔧 | 💡 |
| [no-object-as-default-parameter](docs/rules/no-object-as-default-parameter.md) | Disallow the use of objects as default parameters. || | |
| [no-process-exit](docs/rules/no-process-exit.md) | Disallow `process.exit()`. || | |
| [no-single-promise-in-promise-methods](docs/rules/no-single-promise-in-promise-methods.md) | Disallow passing single-element arrays to `Promise` methods. || 🔧 | 💡 |
| [no-static-only-class](docs/rules/no-static-only-class.md) | Disallow classes that only have static members. || 🔧 | |
| [no-thenable](docs/rules/no-thenable.md) | Disallow `then` property. || | |
| [no-this-assignment](docs/rules/no-this-assignment.md) | Disallow assigning `this` to a variable. || | |
Expand Down
167 changes: 167 additions & 0 deletions rules/no-single-promise-in-promise-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
'use strict';
const {
isCommaToken,
} = require('@eslint-community/eslint-utils');
const {isMethodCall} = require('./ast/index.js');
const {
getParenthesizedText,
isParenthesized,
needsSemicolon,
shouldAddParenthesesToAwaitExpressionArgument,
} = require('./utils/index.js');

const MESSAGE_ID_ERROR = 'no-single-promise-in-promise-methods/error';
const MESSAGE_ID_SUGGESTION_UNWRAP = 'no-single-promise-in-promise-methods/unwrap';
const MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE = 'no-single-promise-in-promise-methods/use-promise-resolve';
const messages = {
[MESSAGE_ID_ERROR]: 'Wrapping single-element array with `Promise.{{method}}()` is unnecessary.',
[MESSAGE_ID_SUGGESTION_UNWRAP]: 'Use the value directly.',
[MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE]: 'Switch to `Promise.resolve(…)`.',
};
const METHODS = ['all', 'any', 'race'];

const isPromiseMethodCallWithSingleElementArray = node =>
isMethodCall(node, {
object: 'Promise',
methods: METHODS,
optionalMember: false,
optionalCall: false,
argumentsLength: 1,
})
&& node.arguments[0].type === 'ArrayExpression'
&& node.arguments[0].elements.length === 1
&& node.arguments[0].elements[0]
&& node.arguments[0].elements[0].type !== 'SpreadElement';

const unwrapAwaitedCallExpression = (callExpression, sourceCode) => fixer => {
const [promiseNode] = callExpression.arguments[0].elements;
let text = getParenthesizedText(promiseNode, sourceCode);

if (
!isParenthesized(promiseNode, sourceCode)
&& shouldAddParenthesesToAwaitExpressionArgument(promiseNode)
) {
text = `(${text})`;
}

// The next node is already behind a `CallExpression`, there should be no ASI problem

return fixer.replaceText(callExpression, text);
};

const unwrapNonAwaitedCallExpression = (callExpression, sourceCode) => fixer => {
const [promiseNode] = callExpression.arguments[0].elements;
let text = getParenthesizedText(promiseNode, sourceCode);

if (
!isParenthesized(promiseNode, sourceCode)
// Since the original call expression can be anywhere, it's hard to tell if the promise
// need to be parenthesized, but it's safe to add parentheses
&& !(
// Known cases that not need parentheses
promiseNode.type === 'Identifier'
|| promiseNode.type === 'MemberExpression'
)
) {
text = `(${text})`;
}

const previousToken = sourceCode.getTokenBefore(callExpression);
if (needsSemicolon(previousToken, sourceCode, text)) {
text = `;${text}`;
}

return fixer.replaceText(callExpression, text);
};

const switchToPromiseResolve = (callExpression, sourceCode) => function * (fixer) {
/*
```
Promise.all([promise,])
// ^^^ methodNameNode
```
*/
const methodNameNode = callExpression.callee.property;
yield fixer.replaceText(methodNameNode, 'resolve');

const [arrayExpression] = callExpression.arguments;
/*
```
Promise.all([promise,])
// ^ openingBracketToken
```
*/
const openingBracketToken = sourceCode.getFirstToken(arrayExpression);
/*
```
Promise.all([promise,])
// ^ penultimateToken
// ^ closingBracketToken
```
*/
const [
penultimateToken,
closingBracketToken,
] = sourceCode.getLastTokens(arrayExpression, 2);

yield fixer.remove(openingBracketToken);
yield fixer.remove(closingBracketToken);

if (isCommaToken(penultimateToken)) {
yield fixer.remove(penultimateToken);
}
};

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => ({
CallExpression(callExpression) {
if (!isPromiseMethodCallWithSingleElementArray(callExpression)) {
return;
}

const problem = {
node: callExpression.arguments[0],
messageId: MESSAGE_ID_ERROR,
data: {
method: callExpression.callee.property.name,
},
};

const {sourceCode} = context;

if (
callExpression.parent.type === 'AwaitExpression'
&& callExpression.parent.argument === callExpression
) {
problem.fix = unwrapAwaitedCallExpression(callExpression, sourceCode);
return problem;
}

problem.suggest = [
{
messageId: MESSAGE_ID_SUGGESTION_UNWRAP,
fix: unwrapNonAwaitedCallExpression(callExpression, sourceCode),
},
{
messageId: MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE,
fix: switchToPromiseResolve(callExpression, sourceCode),
},
];

return problem;
},
});

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
description: 'Disallow passing single-element arrays to `Promise` methods.',
},
fixable: 'code',
hasSuggestions: true,
messages,
},
};
1 change: 1 addition & 0 deletions rules/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module.exports = {
isValueNotUsable: require('./is-value-not-usable.js'),
needsSemicolon: require('./needs-semicolon.js'),
shouldAddParenthesesToMemberExpressionObject: require('./should-add-parentheses-to-member-expression-object.js'),
shouldAddParenthesesToAwaitExpressionArgument: require('./should-add-parentheses-to-await-expression-argument.js'),
singular: require('./singular.js'),
toLocation: require('./to-location.js'),
getAncestor: require('./get-ancestor.js'),
Expand Down
21 changes: 21 additions & 0 deletions rules/utils/should-add-parentheses-to-await-expression-argument.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

/**
Check if parentheses should be added to a `node` when it's used as `argument` of `AwaitExpression`.
@param {Node} node - The AST node to check.
@returns {boolean}
*/
function shouldAddParenthesesToAwaitExpressionArgument(node) {
return (
node.type === 'SequenceExpression'
|| node.type === 'YieldExpression'
|| node.type === 'ArrowFunctionExpression'
|| node.type === 'ConditionalExpression'
|| node.type === 'AssignmentExpression'
|| node.type === 'LogicalExpression'
|| node.type === 'BinaryExpression'
);
}

module.exports = shouldAddParenthesesToAwaitExpressionArgument;
104 changes: 104 additions & 0 deletions test/no-single-promise-in-promise-methods.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import outdent from 'outdent';
import {getTester} from './utils/test.mjs';

const {test} = getTester(import.meta);

// `await`ed
test.snapshot({
valid: [
],
invalid: [
'await Promise.all([(0, promise)])',
'async function * foo() {await Promise.all([yield promise])}',
'async function * foo() {await Promise.all([yield* promise])}',
'await Promise.all([() => promise,],)',
'await Promise.all([a ? b : c,],)',
'await Promise.all([x ??= y,],)',
'await Promise.all([x ||= y,],)',
'await Promise.all([x &&= y,],)',
'await Promise.all([x |= y,],)',
'await Promise.all([x ^= y,],)',
'await Promise.all([x ??= y,],)',
'await Promise.all([x ||= y,],)',
'await Promise.all([x &&= y,],)',
'await Promise.all([x | y,],)',
'await Promise.all([x ^ y,],)',
'await Promise.all([x & y,],)',
'await Promise.all([x !== y,],)',
'await Promise.all([x == y,],)',
'await Promise.all([x in y,],)',
'await Promise.all([x >>> y,],)',
'await Promise.all([x + y,],)',
'await Promise.all([x / y,],)',
'await Promise.all([x ** y,],)',
'await Promise.all([promise,],)',
'await Promise.all([getPromise(),],)',
'await Promise.all([promises[0],],)',
'await Promise.all([await promise])',
'await Promise.any([promise])',
'await Promise.race([promise])',
'await Promise.all([new Promise(() => {})])',
'+await Promise.all([+1])',

// ASI, `Promise.all()` is not really `await`ed
outdent`
await Promise.all([(x,y)])
[0].toString()
`,
],
});

// Not `await`ed
test.snapshot({
valid: [
'Promise.all([promise, anotherPromise])',
'Promise.all(notArrayLiteral)',
'Promise.all([...promises])',
'Promise.any([promise, anotherPromise])',
'Promise.race([promise, anotherPromise])',
'Promise.notListedMethod([promise])',
'Promise[all]([promise])',
'Promise.all([,])',
'NotPromise.all([promise])',
'Promise?.all([promise])',
'Promise.all?.([promise])',
'Promise.all(...[promise])',
'Promise.all([promise], extraArguments)',
'Promise.all()',
'new Promise.all([promise])',

// We are not checking these cases
'globalThis.Promise.all([promise])',
'Promise["all"]([promise])',

// This can't be checked
'Promise.allSettled([promise])',
],
invalid: [
'Promise.all([promise,],)',
outdent`
foo
Promise.all([(0, promise),],)
`,
outdent`
foo
Promise.all([[array][0],],)
`,
'Promise.all([promise]).then()',
'Promise.all([1]).then()',
'Promise.all([1.]).then()',
'Promise.all([.1]).then()',
'Promise.all([(0, promise)]).then()',
'const _ = () => Promise.all([ a ?? b ,],)',
'Promise.all([ {a} = 1 ,],)',
'Promise.all([ function () {} ,],)',
'Promise.all([ class {} ,],)',
'Promise.all([ new Foo ,],).then()',
'Promise.all([ new Foo ,],).toString',
'foo(Promise.all([promise]))',
'Promise.all([promise]).foo = 1',
'Promise.all([promise])[0] ||= 1',
'Promise.all([undefined]).then()',
'Promise.all([null]).then()',
],
});
Loading

0 comments on commit 8f0ee89

Please sign in to comment.