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

feat(rule): add prefer-promise-static-methods rule #473

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 18 additions & 17 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,24 @@ or start with the recommended rule set:
🔧 Automatically fixable by the
[`--fix` CLI option](https://eslint.org/docs/user-guide/command-line-interface#--fix).

| Name                      | Description | 💼 | ⚠️ | 🚫 | 🔧 |
| :------------------------------------------------------------------- | :------------------------------------------------------------------------------------- | :-- | :-- | :-- | :-- |
| [always-return](docs/rules/always-return.md) | Require returning inside each `then()` to create readable and reusable Promise chains. | ✅ | | | |
| [avoid-new](docs/rules/avoid-new.md) | Disallow creating `new` promises outside of utility libs (use [pify][] instead). | | | ✅ | |
| [catch-or-return](docs/rules/catch-or-return.md) | Enforce the use of `catch()` on un-returned promises. | ✅ | | | |
| [no-callback-in-promise](docs/rules/no-callback-in-promise.md) | Disallow calling `cb()` inside of a `then()` (use [nodeify][] instead). | | ✅ | | |
| [no-multiple-resolved](docs/rules/no-multiple-resolved.md) | Disallow creating new promises with paths that resolve multiple times. | | | | |
| [no-native](docs/rules/no-native.md) | Require creating a `Promise` constructor before using it in an ES5 environment. | | | ✅ | |
| [no-nesting](docs/rules/no-nesting.md) | Disallow nested `then()` or `catch()` statements. | | ✅ | | |
| [no-new-statics](docs/rules/no-new-statics.md) | Disallow calling `new` on a Promise static method. | ✅ | | | 🔧 |
| [no-promise-in-callback](docs/rules/no-promise-in-callback.md) | Disallow using promises inside of callbacks. | | ✅ | | |
| [no-return-in-finally](docs/rules/no-return-in-finally.md) | Disallow return statements in `finally()`. | | ✅ | | |
| [no-return-wrap](docs/rules/no-return-wrap.md) | Disallow wrapping values in `Promise.resolve` or `Promise.reject` when not needed. | ✅ | | | |
| [param-names](docs/rules/param-names.md) | Enforce consistent param names and ordering when creating new promises. | ✅ | | | |
| [prefer-await-to-callbacks](docs/rules/prefer-await-to-callbacks.md) | Prefer async/await to the callback pattern. | | | | |
| [prefer-await-to-then](docs/rules/prefer-await-to-then.md) | Prefer `await` to `then()`/`catch()`/`finally()` for reading Promise values. | | | | |
| [valid-params](docs/rules/valid-params.md) | Enforces the proper number of arguments are passed to Promise functions. | | ✅ | | |
| Name                          | Description | 💼 | ⚠️ | 🚫 | 🔧 |
| :--------------------------------------------------------------------------- | :------------------------------------------------------------------------------------- | :-- | :-- | :-- | :-- |
| [always-return](docs/rules/always-return.md) | Require returning inside each `then()` to create readable and reusable Promise chains. | ✅ | | | |
| [avoid-new](docs/rules/avoid-new.md) | Disallow creating `new` promises outside of utility libs (use [pify][] instead). | | | ✅ | |
| [catch-or-return](docs/rules/catch-or-return.md) | Enforce the use of `catch()` on un-returned promises. | ✅ | | | |
| [no-callback-in-promise](docs/rules/no-callback-in-promise.md) | Disallow calling `cb()` inside of a `then()` (use [nodeify][] instead). | | ✅ | | |
| [no-multiple-resolved](docs/rules/no-multiple-resolved.md) | Disallow creating new promises with paths that resolve multiple times. | | | | |
| [no-native](docs/rules/no-native.md) | Require creating a `Promise` constructor before using it in an ES5 environment. | | | ✅ | |
| [no-nesting](docs/rules/no-nesting.md) | Disallow nested `then()` or `catch()` statements. | | ✅ | | |
| [no-new-statics](docs/rules/no-new-statics.md) | Disallow calling `new` on a Promise static method. | ✅ | | | 🔧 |
| [no-promise-in-callback](docs/rules/no-promise-in-callback.md) | Disallow using promises inside of callbacks. | | ✅ | | |
| [no-return-in-finally](docs/rules/no-return-in-finally.md) | Disallow return statements in `finally()`. | | ✅ | | |
| [no-return-wrap](docs/rules/no-return-wrap.md) | Disallow wrapping values in `Promise.resolve` or `Promise.reject` when not needed. | ✅ | | | |
| [param-names](docs/rules/param-names.md) | Enforce consistent param names and ordering when creating new promises. | ✅ | | | |
| [prefer-await-to-callbacks](docs/rules/prefer-await-to-callbacks.md) | Prefer async/await to the callback pattern. | | | | |
| [prefer-await-to-then](docs/rules/prefer-await-to-then.md) | Prefer `await` to `then()`/`catch()`/`finally()` for reading Promise values. | | | | |
| [prefer-promise-static-methods](docs/rules/prefer-promise-static-methods.md) | Prefer `Promise.resolve(foo)` to `new Promise((resolve) => resolve(foo))`. | | | | 🔧 |
| [valid-params](docs/rules/valid-params.md) | Enforces the proper number of arguments are passed to Promise functions. | | ✅ | | |

<!-- end auto-generated rules list -->

Expand Down
69 changes: 69 additions & 0 deletions __tests__/prefer-promise-static-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
'use strict'

const rule = require('../rules/prefer-promise-static-methods')
const { RuleTester } = require('./rule-tester')
const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 6,
},
})

const resolveErrorMessage =
'Prefer `Promise.resolve()` to `new Promise()`. The static method is faster, more readable and less verbose.'
const rejectErrorMessage =
'Prefer `Promise.reject()` to `new Promise()`. The static method is faster, more readable and less verbose.'

ruleTester.run('prefer-promise-static-methods', rule, {
valid: [
`Promise.resolve(foo)`,
`Promise.reject(bar)`,
`new Promise(() => {})`,
`new Promise((resolve) => setTimeout(resolve, 100))`,
`new Promise(process.nextTick)`,
`new Promise((resolve) => { resolve(foo); resolve(bar) })`,
`new Promise((resolve, reject) => { foo(bar) })`,
`new Promise((resolve, reject) => { foo && resolve(bar) })`,
`new Promise((...args) => {})`,
// This is a type error but the rule wouldn't check it
`new Promise(([foo, bar]) => {})`,
`new Promise(([foo, bar] = []) => {})`,
],

invalid: [
{
code: `new Promise((resolve) => resolve(foo))`,
output: `Promise.resolve(foo)`,
errors: [{ message: resolveErrorMessage }],
},
{
code: `new Promise((resolve) => { resolve(foo) })`,
output: `Promise.resolve(foo)`,
errors: [{ message: resolveErrorMessage }],
},
{
code: `new Promise((resolve) => resolve())`,
output: `Promise.resolve()`,
errors: [{ message: resolveErrorMessage }],
},
{
code: `new Promise((_, reject) => reject(foo))`,
output: `Promise.reject(foo)`,
errors: [{ message: rejectErrorMessage }],
},
{
code: `new Promise((resolve, reject) => { reject(foo) })`,
output: `Promise.reject(foo)`,
errors: [{ message: rejectErrorMessage }],
},
{
code: `new Promise(function foo(resolve, reject) { reject(bar) })`,
output: `Promise.reject(bar)`,
errors: [{ message: rejectErrorMessage }],
},
{
code: `new Promise((resolve = unusedDefault) => resolve())`,
output: `Promise.resolve()`,
errors: [{ message: resolveErrorMessage }],
},
],
})
42 changes: 42 additions & 0 deletions docs/rules/prefer-promise-static-methods.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Prefer `Promise.resolve(foo)` to `new Promise((resolve) => resolve(foo))` (`promise/prefer-promise-static-methods`)

🔧 This rule is automatically fixable by the
[`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->

Using `new Promise()` for simple use cases is a common mistake when developers
first start working with promises. When `resolve` or `reject` is immediately
called in the promise executor, it's better to just use the static
`Promise.resolve()` or `Promise.reject()` methods. The static methods are
faster, more readable and less verbose.

#### Valid

```js
Promise.resolve(foo)
Promise.reject(bar)

new Promise((resolve) => setTimeout(resolve, 100))
new Promise((resolve, reject) => {
if (foo) {
resolve(bar)
}
})
```

#### Invalid

```js
new Promise((resolve) => resolve(foo))
new Promise((resolve) => {
resolve(foo)
})
// autofix to Promise.resolve(foo);

new Promise((_, reject) => reject(foo))
new Promise(function (_, reject) {
reject(foo)
})
// autofix to Promise.reject(foo);
```
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module.exports = {
'catch-or-return': require('./rules/catch-or-return'),
'prefer-await-to-callbacks': require('./rules/prefer-await-to-callbacks'),
'prefer-await-to-then': require('./rules/prefer-await-to-then'),
'prefer-promise-static-methods': require('./rules/prefer-promise-static-methods'),
'no-native': require('./rules/no-native'),
'no-callback-in-promise': require('./rules/no-callback-in-promise'),
'no-promise-in-callback': require('./rules/no-promise-in-callback'),
Expand Down
131 changes: 131 additions & 0 deletions rules/prefer-promise-static-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/**
* Rule: prefer-promise-static-methods
* Prefer `Promise.resolve(foo)` to `new Promise((resolve) => resolve(foo))`.
*/

'use strict'

const { getSourceCode } = require('./lib/eslint-compat')
const getDocsUrl = require('./lib/get-docs-url')
const {
isPromiseConstructorWithInlineExecutor,
} = require('./lib/is-promise-constructor')

module.exports = {
meta: {
type: 'suggestion',
docs: {
description:
'Prefer `Promise.resolve(foo)` to `new Promise((resolve) => resolve(foo))`.',
url: getDocsUrl('prefer-promise-static-methods'),
},
messages: {
replaceWithStaticMethod:
'Prefer `Promise.{{ method }}()` to `new Promise()`. The static method is faster, more readable and less verbose.',
},
fixable: 'code',
schema: [],
},
create(context) {
const sourceCode = getSourceCode(context)
/**
* Report an error if the given node is a call to `resolve` or `reject`.
* @param {import('estree').SimpleCallExpression} callNode
* @param {import('estree').NewExpression} constructorNode
* @param {Array<string | undefined>} parameterNames
*/
function reportIfIsPromiseCall(callNode, constructorNode, parameterNames) {
if (
callNode.callee.type === 'Identifier' &&
callNode.arguments.length <= 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about new Promise(resolve => resolve(itCanThrowAnError())) (the case of Promise.try)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure what the best approach there would be. (Also I didn't realize new Promise is same tick, I must've misread the spec.) Perhaps just show a suggestion instead of an autofix?

Copy link
Contributor

@zloirock zloirock May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cases that could throw an error should be ignored (however, it's too many possible cases - I think that it's OK to ignore only call and new). Also, can be added an option to check such cases, without autofixes.

) {
/** @type {'resolve' | 'reject'} */
let method
if (callNode.callee.name === parameterNames[0]) {
method = 'resolve'
} else if (callNode.callee.name === parameterNames[1]) {
method = 'reject'
} else {
return
}

// Value passed to resolve/reject method
const valueNode = callNode.arguments[0]
const valueText = valueNode
? sourceCode.getText(callNode.arguments[0])
: ''
context.report({
node: callNode,
messageId: 'replaceWithStaticMethod',
data: { method },
fix: (fixer) =>
fixer.replaceText(
constructorNode,
`Promise.${method}(${valueText})`
),
})
}
}

return {
NewExpression(node) {
if (isPromiseConstructorWithInlineExecutor(node)) {
const func = node.arguments[0]
const parameterNames = getParameterNames(func.params)

if (func.body.type === 'CallExpression') {
// (resolve) => resolve(foo)
reportIfIsPromiseCall(func.body, node, parameterNames)
} else if (
func.body.type === 'BlockStatement' &&
func.body.body.length === 1
) {
const statement = func.body.body[0]
if (
statement.type === 'ExpressionStatement' &&
statement.expression.type === 'CallExpression'
) {
// (resolve) => { resolve(foo) }
reportIfIsPromiseCall(statement.expression, node, parameterNames)
}
}
}
},
}
},
}

/**
* Given AST for `(resolve, reject) => {...}` params, return `['resolve', 'reject']`.
* @param {import('estree').Pattern[]} params
*/
function getParameterNames(params) {
/** @type {Array<string | undefined>} */
const names = []
for (const param of params) {
switch (param.type) {
// (resolve) =>
case 'Identifier':
names.push(param.name)
break
// (resolve = foo) =>
case 'AssignmentPattern':
if (param.left.type === 'Identifier') {
names.push(param.left.name)
} else {
names.push(undefined)
}
break
// (...args) =>
case 'RestElement':
// there won't be any more valid names
return names
// ([resolve]) =>
default:
names.push(undefined)
break
}
}

return names
}
Loading