Skip to content

no-array-for-each: Ignore React.Children.forEach #1088

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

Merged
merged 13 commits into from
Feb 6, 2021
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions rules/no-array-callback-reference.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {isParenthesized} = require('eslint-utils');
const getDocumentationUrl = require('./utils/get-documentation-url');
const methodSelector = require('./utils/method-selector');
const {notFunctionSelector} = require('./utils/not-function');
const isNodeMatches = require('./utils/is-node-matches');

const ERROR_WITH_NAME_MESSAGE_ID = 'error-with-name';
const ERROR_WITHOUT_NAME_MESSAGE_ID = 'error-without-name';
Expand Down Expand Up @@ -70,7 +71,7 @@ const iteratorMethods = [

const ignoredCallee = [
'Promise',
'React.children',
'React.Children',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this mean to be React.Children.

'Children',
'lodash',
'underscore',
Expand All @@ -79,18 +80,6 @@ const ignoredCallee = [
'async'
];

const toSelector = name => {
const splitted = name.split('.');
return `[callee.${'object.'.repeat(splitted.length)}name!="${splitted.shift()}"]`;
};

// Select all the call expressions except the ones present in the ignore list.
const ignoredCalleeSelector = [
// `this.{map, filter, …}()`
'[callee.object.type!="ThisExpression"]',
...ignoredCallee.map(name => toSelector(name))
].join('');

function check(context, node, method, options) {
const {type} = node;

Expand Down Expand Up @@ -161,11 +150,17 @@ const create = context => {
max: 2
}),
options.extraSelector,
ignoredCalleeSelector,
ignoredFirstArgumentSelector
].join('');

rules[selector] = node => {
if (
isNodeMatches(node.callee.object, ignoredCallee) ||
node.callee.object.type === 'ThisExpression'
) {
return;
}

const [iterator] = node.arguments;
check(context, iterator, method, options, sourceCode);
};
Expand Down
10 changes: 10 additions & 0 deletions rules/no-array-for-each.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const shouldAddParenthesesToExpressionStatementExpression = require('./utils/sho
const getParenthesizedTimes = require('./utils/get-parenthesized-times');
const extendFixRange = require('./utils/extend-fix-range');
const isFunctionSelfUsedInside = require('./utils/is-function-self-used-inside');
const isNodeMatches = require('./utils/is-node-matches');

const MESSAGE_ID = 'no-array-for-each';
const messages = {
Expand Down Expand Up @@ -315,6 +316,11 @@ function isFixable(callExpression, sourceCode, {scope, functionInfo, allIdentifi
return true;
}

const ignoredObjects = [
'React.Children',
'Children'
];
Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking we could have a shared utility method that is like isNotNativeArrayMethod. In case there are other false-positives in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good idea, like the not-dom-node.js, but I was hoping get some help from eslint-utils, anyway I'll keep this in mind, maybe try in another PR.

Copy link
Owner

Choose a reason for hiding this comment

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

That package isn't very actively maintained. I suggest we just do the utilities here for now and we can move them there later on if it becomes maintained again. Alternatively, you could fork eslint-utils for now.


const create = context => {
const functionStack = [];
const callExpressions = [];
Expand Down Expand Up @@ -349,6 +355,10 @@ const create = context => {
returnStatements.push(node);
},
[arrayForEachCallSelector](node) {
if (isNodeMatches(node.callee.object, ignoredObjects)) {
return;
}

callExpressions.push({
node,
scope: context.getScope()
Expand Down
36 changes: 36 additions & 0 deletions rules/utils/is-node-matches.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

function isNodeMatchesNameOrPath(node, nameOrPath) {
const names = nameOrPath.split('.');
for (let index = names.length - 1; index >= 0; index--) {
const name = names[index];

if (index === 0) {
return node.type === 'Identifier' && node.name === name;
}

if (
node.type !== 'MemberExpression' ||
node.optional ||
node.property.type !== 'Identifier' ||
node.property.name !== name
) {
return false;
}

node = node.object;
}
}

/**
Check if node matches any object name or key path.

@param {Node} node - The AST node to check.
@param {string[]} nameOrPaths - The object name or key paths.
@returns {boolean}
*/
function isNodeMatches(node, nameOrPaths) {
return nameOrPaths.some(nameOrPath => isNodeMatchesNameOrPath(node, nameOrPath));
}

module.exports = isNodeMatches;
12 changes: 10 additions & 2 deletions test/no-array-for-each.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ test.snapshot({
valid: [
'new foo.forEach(element => bar())',
'forEach(element => bar())',
'foo.notForEach(element => bar())'
'foo.notForEach(element => bar())',
// #1087
'React.Children.forEach(children, (child) => {});',
'Children.forEach(children, (child) => {});'
],
invalid: [
// Not fixable
Expand Down Expand Up @@ -334,7 +337,12 @@ test.snapshot({
bar() {}
}
].forEach((Foo, bar) => process(Foo, bar))
`
`,
'foo.React.Children.forEach(bar)',
'NotReact.Children.forEach(bar)',
'React.NotChildren.forEach(bar)',
'React?.Children.forEach(bar)',
'NotChildren.forEach(bar)'
]
});

Expand Down
50 changes: 50 additions & 0 deletions test/snapshots/no-array-for-each.js.md
Original file line number Diff line number Diff line change
Expand Up @@ -1404,3 +1404,53 @@ Generated by [AVA](https://avajs.dev).
> 5 | ].forEach((Foo, bar) => process(Foo, bar))␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #77
1 | foo.React.Children.forEach(bar)

> Error 1/1

`␊
> 1 | foo.React.Children.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #78
1 | NotReact.Children.forEach(bar)

> Error 1/1

`␊
> 1 | NotReact.Children.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #79
1 | React.NotChildren.forEach(bar)

> Error 1/1

`␊
> 1 | React.NotChildren.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #80
1 | React?.Children.forEach(bar)

> Error 1/1

`␊
> 1 | React?.Children.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #81
1 | NotChildren.forEach(bar)

> Error 1/1

`␊
> 1 | NotChildren.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`
Binary file modified test/snapshots/no-array-for-each.js.snap
Binary file not shown.