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

prefer-array-find: Prefer .findLast() #1900

Merged
merged 10 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 36 additions & 2 deletions docs/rules/prefer-array-find.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Prefer `.find(…)` over the first element from `.filter(…)`
# Prefer `.find(…)` and `.findLast(…)` over the first or last element from `.filter(…)`

<!-- Do not manually modify RULE_NOTICE part. Run: `npm run generate-rule-notices` -->
<!-- RULE_NOTICE -->
Expand All @@ -7,7 +7,7 @@
🔧💡 *This rule is [auto-fixable](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) and provides [suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).*
<!-- /RULE_NOTICE -->

[`Array#find()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find) breaks the loop as soon as it finds a match and doesn't create a new array.
[`Array#find()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find) and [`Array#findLast()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLast) breaks the loop as soon as it finds a match and doesn't create a new array.

This rule is fixable unless default values are used in declaration or assignment.

Expand Down Expand Up @@ -38,3 +38,37 @@ const item = array.find(x => isUnicorn(x));
```js
item = array.find(x => isUnicorn(x));
```

```js
const item = array.findLast(x => isUnicorn(x));
```

## Options

Type: `object`

### checkFromLast

Type: `boolean`\
Default: `false`

Pass `checkFromLast: true` to check cases searching from last.

#### Fail

```js
// eslint unicorn/prefer-array-find: ["error", {"checkFromLast": true}]
const item = array.filter(x => isUnicorn(x)).at(-1);
```

```js
// eslint unicorn/prefer-array-find: ["error", {"checkFromLast": true}]
const item = array.filter(x => isUnicorn(x)).pop();
```

#### Pass

```js
// eslint unicorn/prefer-array-find: ["error", {"checkFromLast": true}]
const item = array.findLast(x => isUnicorn(x));
```
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ Each rule has emojis denoting:
| [number-literal-case](docs/rules/number-literal-case.md) | Enforce proper case for numeric literals. | ✅ | 🔧 | |
| [numeric-separators-style](docs/rules/numeric-separators-style.md) | Enforce the style of numeric separators by correctly grouping digits. | ✅ | 🔧 | |
| [prefer-add-event-listener](docs/rules/prefer-add-event-listener.md) | Prefer `.addEventListener()` and `.removeEventListener()` over `on`-functions. | ✅ | 🔧 | |
| [prefer-array-find](docs/rules/prefer-array-find.md) | Prefer `.find(…)` over the first element from `.filter(…)`. | ✅ | 🔧 | 💡 |
| [prefer-array-find](docs/rules/prefer-array-find.md) | Prefer `.find(…)` and `.findLast(…)` over the first or last element from `.filter(…)`. | ✅ | 🔧 | 💡 |
| [prefer-array-flat](docs/rules/prefer-array-flat.md) | Prefer `Array#flat()` over legacy techniques to flatten arrays. | ✅ | 🔧 | |
| [prefer-array-flat-map](docs/rules/prefer-array-flat-map.md) | Prefer `.flatMap(…)` over `.map(…).flat()`. | ✅ | 🔧 | |
| [prefer-array-index-of](docs/rules/prefer-array-index-of.md) | Prefer `Array#indexOf()` over `Array#findIndex()` when looking for the index of an item. | ✅ | 🔧 | 💡 |
Expand Down
83 changes: 81 additions & 2 deletions rules/prefer-array-find.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ const {

const ERROR_ZERO_INDEX = 'error-zero-index';
const ERROR_SHIFT = 'error-shift';
const ERROR_POP = 'error-pop';
const ERROR_AT_MINUS_ONE = 'error-at-minus-one';
const ERROR_DESTRUCTURING_DECLARATION = 'error-destructuring-declaration';
const ERROR_DESTRUCTURING_ASSIGNMENT = 'error-destructuring-assignment';
const ERROR_DECLARATION = 'error-variable';
Expand All @@ -27,6 +29,8 @@ const messages = {
[ERROR_DECLARATION]: 'Prefer `.find(…)` over `.filter(…)`.',
[ERROR_ZERO_INDEX]: 'Prefer `.find(…)` over `.filter(…)[0]`.',
[ERROR_SHIFT]: 'Prefer `.find(…)` over `.filter(…).shift()`.',
[ERROR_POP]: 'Prefer `.findLast(…)` over `.filter(…).pop()`.',
[ERROR_AT_MINUS_ONE]: 'Prefer `.findLast(…)` over `.filter(…).at(-1)`.',
[ERROR_DESTRUCTURING_DECLARATION]: 'Prefer `.find(…)` over destructuring `.filter(…)`.',
// Same message as `ERROR_DESTRUCTURING_DECLARATION`, but different case
[ERROR_DESTRUCTURING_ASSIGNMENT]: 'Prefer `.find(…)` over destructuring `.filter(…)`.',
Expand Down Expand Up @@ -76,6 +80,33 @@ const shiftSelector = [
}),
].join('');

const popSelector = [
methodCallSelector({
method: 'pop',
argumentsLength: 0,
}),
methodCallSelector({
...filterMethodSelectorOptions,
path: 'callee.object',
}),
].join('');

const atMinusOneSelector = [
methodCallSelector({
method: 'at',
argumentsLength: 1,
}),
'[arguments.0.type="UnaryExpression"]',
'[arguments.0.operator="-"]',
'[arguments.0.prefix]',
'[arguments.0.argument.type="Literal"]',
'[arguments.0.argument.raw=1]',
methodCallSelector({
...filterMethodSelectorOptions,
path: 'callee.object',
}),
].join('');

const destructuringDeclaratorSelector = [
'VariableDeclarator',
'[id.type="ArrayPattern"]',
Expand Down Expand Up @@ -229,8 +260,14 @@ const isDestructuringFirstElement = node => {
/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
const sourceCode = context.getSourceCode();
const {
checkFromLast,
} = {
checkFromLast: false,
...context.options[0],
};

return {
const listeners = {
[zeroIndexSelector](node) {
return {
node: node.object.callee.property,
Expand Down Expand Up @@ -319,18 +356,60 @@ const create = context => {
return problem;
},
};

if (!checkFromLast) {
return listeners;
}

return Object.assign(listeners, {
[popSelector](node) {
return {
node: node.callee.object.callee.property,
messageId: ERROR_POP,
fix: fixer => [
fixer.replaceText(node.callee.object.callee.property, 'findLast'),
...removeMethodCall(fixer, node, sourceCode),
],
};
},
[atMinusOneSelector](node) {
return {
node: node.callee.object.callee.property,
messageId: ERROR_AT_MINUS_ONE,
fix: fixer => [
fixer.replaceText(node.callee.object.callee.property, 'findLast'),
...removeMethodCall(fixer, node, sourceCode),
],
};
},
});
};

const schema = [
{
type: 'object',
additionalProperties: false,
properties: {
checkFromLast: {
type: 'boolean',
// TODO: Change default value to `true`, or remove the option when targeting Node.js 18.
default: false,
},
},
},
];

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
description: 'Prefer `.find(…)` over the first element from `.filter(…)`.',
description: 'Prefer `.find(…)` and `.findLast(…)` over the first or last element from `.filter(…)`.',
},
fixable: 'code',
hasSuggestions: true,
schema,
messages,
},
};
181 changes: 181 additions & 0 deletions test/prefer-array-find.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const {test} = getTester(import.meta);

const ERROR_ZERO_INDEX = 'error-zero-index';
const ERROR_SHIFT = 'error-shift';
const ERROR_POP = 'error-pop';
const ERROR_AT_MINUS_ONE = 'error-at-minus-one';
const ERROR_DESTRUCTURING_DECLARATION = 'error-destructuring-declaration';
const ERROR_DESTRUCTURING_ASSIGNMENT = 'error-destructuring-assignment';
const ERROR_DECLARATION = 'error-variable';
Expand Down Expand Up @@ -914,3 +916,182 @@ test({
},
],
});

// Check from last
const checkFromLastOptions = [{checkFromLast: true}];

// Default to false
test({
valid: [
'array.filter(foo).pop()',
'array.filter(foo).at(-1)',
],
invalid: [],
});

// `.pop()`
test({
valid: [
// Test `.pop()`
// Not `CallExpression`
'array.filter(foo).pop',
// Not `MemberExpression`
'pop(array.filter(foo))',
// `callee.property` is not a `Identifier`
'array.filter(foo)["pop"]()',
// Computed
'array.filter(foo)[pop]()',
// Not `pop`
'array.filter(foo).notPop()',
// More or less argument(s)
'array.filter(foo).pop(extraArgument)',
'array.filter(foo).pop(...[])',

// Test `.filter()`
// Not `CallExpression`
'array.filter.pop()',
// Not `MemberExpression`
'filter(foo).pop()',
// `callee.property` is not a `Identifier`
'array["filter"](foo).pop()',
// Computed
'array[filter](foo).pop()',
// Not `filter`
'array.notFilter(foo).pop()',
// More or less argument(s)
'array.filter().pop()',
'array.filter(foo, thisArgument, extraArgument).pop()',
'array.filter(...foo).pop()',
].map(code => ({code, options: checkFromLastOptions})),
invalid: [
{
code: 'array.filter(foo).pop()',
output: 'array.findLast(foo)',
errors: [{messageId: ERROR_POP}],
},
{
code: 'array.filter(foo, thisArgument).pop()',
output: 'array.findLast(foo, thisArgument)',
errors: [{messageId: ERROR_POP}],
},
{
code: outdent`
const item = array
// comment 1
.filter(
// comment 2
x => x === '🦄'
)
// comment 3
.pop()
// comment 4
;
`,
output: outdent`
const item = array
// comment 1
.findLast(
// comment 2
x => x === '🦄'
)
// comment 4
;
`,
errors: [{messageId: ERROR_POP}],
},
].map(test => ({...test, options: checkFromLastOptions})),
});

// `.at(-1)`
test({
valid: [
// Test `.at()`
// Not `CallExpression`
'array.filter(foo).at',
// Not `MemberExpression`
'at(array.filter(foo), -1)',
// `callee.property` is not a `Identifier`
'array.filter(foo)["at"](-1)',
// Computed
'array.filter(foo)[at](-1)',
// Not `at`
'array.filter(foo).notAt(-1)',
// More or less argument(s)
'array.filter(foo).at()',
'array.filter(foo).at(-1, extraArgument)',
'array.filter(foo).at(...[-1])',

// Test `-1`
'array.filter(foo).at(1)',
'array.filter(foo).at(+1)',
'const ONE = 1; array.filter(foo).at(-ONE)',
'const MINUS_ONE = 1; array.filter(foo).at(MINUS_ONE)',
'const a = {b: 1}; array.filter(foo).at(-a.b)',
'const a = {b: -1}; array.filter(foo).at(a.b)',
'array.filter(foo).at(-2)',
'array.filter(foo).at(-(-1))',
'array.filter(foo).at(-1.)',
'array.filter(foo).at(-0b1)',
'array.filter(foo).at(-"1")',
'array.filter(foo).at(-null)',
'array.filter(foo).at(-false)',
'array.filter(foo).at(-true)',

// Test `.filter()`
// Not `CallExpression`
'array.filter.at(-1)',
// Not `MemberExpression`
'filter(foo).at(-1)',
// `callee.property` is not a `Identifier`
'array["filter"](foo).at(-1)',
// Computed
'array[filter](foo).at(-1)',
// Not `filter`
'array.notFilter(foo).at(-1)',
// More or less argument(s)
'array.filter().at(-1)',
'array.filter(foo, thisArgument, extraArgument).at(-1)',
'array.filter(...foo).at(-1)',
].map(code => ({code, options: checkFromLastOptions})),
invalid: [
{
code: 'array.filter(foo).at(-1)',
output: 'array.findLast(foo)',
errors: [{messageId: ERROR_AT_MINUS_ONE}],
},
{
code: 'array.filter(foo, thisArgument).at(-1)',
output: 'array.findLast(foo, thisArgument)',
errors: [{messageId: ERROR_AT_MINUS_ONE}],
},
{
code: outdent`
const item = array
// comment 1
.filter(
// comment 2
x => x === '🦄'
)
// comment 3
.at(
// comment 4
-1
// comment 5
)
// comment 6
;
`,
output: outdent`
const item = array
// comment 1
.findLast(
// comment 2
x => x === '🦄'
)
// comment 6
;
`,
errors: [{messageId: ERROR_AT_MINUS_ONE}],
},
].map(test => ({...test, options: checkFromLastOptions})),
});