Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add no-single-promise-in-promise-methods rule #2258

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
38306bc
Add `no-single-promise-in-promise-methods` rule
Clement398 Jan 16, 2024
a728a1a
Use snapshot test
fisker Jan 27, 2024
7030d7f
More tests
fisker Jan 27, 2024
1f24588
Refactor
fisker Jan 27, 2024
92454c7
More tests
fisker Jan 27, 2024
a2647ae
Not limit types
Clement398 Jan 28, 2024
7869878
Add leading semicolon
Clement398 Jan 29, 2024
60467a6
Add tests
Clement398 Jan 29, 2024
26e557a
Update error message
Clement398 Jan 31, 2024
8417930
Update error message
Clement398 Feb 3, 2024
345b637
Rename functions
Clement398 Feb 3, 2024
a32688f
Fix .then() error
Clement398 Feb 3, 2024
8893487
Update no-single-promise-in-promise-methods.md
sindresorhus Feb 22, 2024
e28b11a
Update tests
Clement398 Feb 22, 2024
e15154e
Update description message
Clement398 Feb 22, 2024
d74d45f
Update no-single-promise-in-promise-methods.md and tests
Clement398 Feb 22, 2024
377da6e
Fix exponentiation operator error
Clement398 Feb 22, 2024
ebd4708
Add `shouldAddParenthesesToAwaitExpressionArgument`
fisker Feb 23, 2024
062002e
Add todo
fisker Feb 23, 2024
8ed5eda
Fix
fisker Feb 23, 2024
548f94f
Rename message id
fisker Feb 23, 2024
a32c16c
Improve `switchToPromiseResolve`
fisker Feb 23, 2024
b5d870c
Improve `unwrapNonAwaitedCallExpression`
fisker Feb 23, 2024
86b3b4b
Update message
fisker Feb 23, 2024
5a1232c
Stricter
fisker Feb 23, 2024
bd06124
Tweak tests
fisker Feb 23, 2024
72f0a13
More tests
fisker Feb 23, 2024
d42c1d7
Update docs
fisker Feb 23, 2024
04bf100
More tests
fisker Feb 23, 2024
5e82ac7
Update message
fisker Feb 23, 2024
5799fd3
Update docs
fisker Feb 23, 2024
ca335f6
Loosely hole check
fisker Feb 23, 2024
eed65b6
More tests
fisker Feb 23, 2024
c97843e
More
fisker Feb 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
};
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved

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
Loading