Skip to content

Commit 9a8a4b5

Browse files
Wesitosljharb
Wesitos
authored and
ljharb
committed
[Fix] component detection: use estraverse to improve component detection
Fixes #2960.
1 parent 291acbf commit 9a8a4b5

File tree

9 files changed

+351
-90
lines changed

9 files changed

+351
-90
lines changed

‎CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
55

66
## Unreleased
77

8+
### Fixed
9+
* component detection: use `estraverse` to improve component detection ([#2992][] @Wesitos)
10+
11+
[#2992]: https://github.com/yannickcr/eslint-plugin-react/pull/2992
12+
813
## [7.24.0] - 2021.05.27
914

1015
### Added

‎lib/rules/no-typos.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -241,8 +241,9 @@ module.exports = {
241241

242242
if (
243243
relatedComponent
244-
&& (utils.isES6Component(relatedComponent.node) || utils.isReturningJSX(relatedComponent.node))
245-
&& (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
244+
&& (utils.isES6Component(relatedComponent.node) || (
245+
relatedComponent.node.type !== 'ClassDeclaration' && utils.isReturningJSX(relatedComponent.node)))
246+
&& (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
246247
) {
247248
reportErrorIfPropertyCasingTypo(node.parent.right, node.property, true);
248249
}

‎lib/util/Components.js

Lines changed: 15 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -47,36 +47,6 @@ function mergeUsedPropTypes(propsList, newPropsList) {
4747
return propsList.concat(propsToAdd);
4848
}
4949

50-
function isReturnsConditionalJSX(node, property, strict) {
51-
const returnsConditionalJSXConsequent = node[property]
52-
&& node[property].type === 'ConditionalExpression'
53-
&& jsxUtil.isJSX(node[property].consequent);
54-
const returnsConditionalJSXAlternate = node[property]
55-
&& node[property].type === 'ConditionalExpression'
56-
&& jsxUtil.isJSX(node[property].alternate);
57-
return strict
58-
? (returnsConditionalJSXConsequent && returnsConditionalJSXAlternate)
59-
: (returnsConditionalJSXConsequent || returnsConditionalJSXAlternate);
60-
}
61-
62-
function isReturnsLogicalJSX(node, property, strict) {
63-
const returnsLogicalJSXLeft = node[property]
64-
&& node[property].type === 'LogicalExpression'
65-
&& jsxUtil.isJSX(node[property].left);
66-
const returnsLogicalJSXRight = node[property]
67-
&& node[property].type === 'LogicalExpression'
68-
&& jsxUtil.isJSX(node[property].right);
69-
return strict
70-
? (returnsLogicalJSXLeft && returnsLogicalJSXRight)
71-
: (returnsLogicalJSXLeft || returnsLogicalJSXRight);
72-
}
73-
74-
function isReturnsSequentialJSX(node, property) {
75-
return node[property]
76-
&& node[property].type === 'SequenceExpression'
77-
&& jsxUtil.isJSX(node[property].expressions[node[property].expressions.length - 1]);
78-
}
79-
8050
const Lists = new WeakMap();
8151

8252
/**
@@ -466,65 +436,12 @@ function componentRule(rule, context) {
466436
};
467437
},
468438

469-
/**
470-
* Check if the node is returning JSX
471-
*
472-
* @param {ASTNode} ASTnode The AST node being checked
473-
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
474-
* @returns {Boolean} True if the node is returning JSX, false if not
475-
*/
476-
isReturningJSX(ASTnode, strict) {
477-
const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
478-
const node = nodeAndProperty.node;
479-
const property = nodeAndProperty.property;
480-
481-
if (!node) {
482-
return false;
483-
}
484-
485-
const returnsConditionalJSX = isReturnsConditionalJSX(node, property, strict);
486-
const returnsLogicalJSX = isReturnsLogicalJSX(node, property, strict);
487-
const returnsSequentialJSX = isReturnsSequentialJSX(node, property);
488-
489-
const returnsJSX = node[property] && jsxUtil.isJSX(node[property]);
490-
const returnsPragmaCreateElement = this.isCreateElement(node[property]);
491-
492-
return !!(
493-
returnsConditionalJSX
494-
|| returnsLogicalJSX
495-
|| returnsSequentialJSX
496-
|| returnsJSX
497-
|| returnsPragmaCreateElement
498-
);
439+
isReturningJSX(ASTNode, strict) {
440+
return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict, true);
499441
},
500442

501-
/**
502-
* Check if the node is returning null
503-
*
504-
* @param {ASTNode} ASTnode The AST node being checked
505-
* @returns {Boolean} True if the node is returning null, false if not
506-
*/
507-
isReturningNull(ASTnode) {
508-
const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
509-
const property = nodeAndProperty.property;
510-
const node = nodeAndProperty.node;
511-
512-
if (!node) {
513-
return false;
514-
}
515-
516-
return node[property] && node[property].value === null;
517-
},
518-
519-
/**
520-
* Check if the node is returning JSX or null
521-
*
522-
* @param {ASTNode} ASTNode The AST node being checked
523-
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
524-
* @returns {Boolean} True if the node is returning JSX or null, false if not
525-
*/
526443
isReturningJSXOrNull(ASTNode, strict) {
527-
return utils.isReturningJSX(ASTNode, strict) || utils.isReturningNull(ASTNode);
444+
return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict);
528445
},
529446

530447
getPragmaComponentWrapper(node) {
@@ -734,9 +651,20 @@ function componentRule(rule, context) {
734651
return undefined;
735652
}
736653
if (utils.isInAllowedPositionForComponent(node) && utils.isReturningJSXOrNull(node)) {
737-
if (!node.id || isFirstLetterCapitalized(node.id.name)) {
654+
const isMethod = node.parent.type === 'Property' && node.parent.method;
655+
656+
if (isMethod && !isFirstLetterCapitalized(node.parent.key.name)) {
657+
return utils.isReturningJSX(node) ? node : undefined;
658+
}
659+
660+
if (node.id && isFirstLetterCapitalized(node.id.name)) {
738661
return node;
739662
}
663+
664+
if (!node.id) {
665+
return node;
666+
}
667+
740668
return undefined;
741669
}
742670

‎lib/util/ast.js

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,29 @@
44

55
'use strict';
66

7+
const estraverse = require('estraverse');
8+
9+
/**
10+
* Wrapper for estraverse.traverse
11+
*
12+
* @param {ASTNode} ASTnode The AST node being checked
13+
* @param {Object} visitor Visitor Object for estraverse
14+
*/
15+
function traverse(ASTnode, visitor) {
16+
const opts = Object.assign({}, {
17+
fallback(node) {
18+
return Object.keys(node).filter((key) => key === 'children' || key === 'argument');
19+
}
20+
}, visitor);
21+
22+
opts.keys = Object.assign({}, visitor.keys, {
23+
JSXElement: ['children'],
24+
JSXFragment: ['children']
25+
});
26+
27+
estraverse.traverse(ASTnode, opts);
28+
}
29+
730
/**
831
* Find a return statment in the current node
932
*
@@ -37,6 +60,49 @@ function findReturnStatement(node) {
3760
}(bodyNodes));
3861
}
3962

63+
/**
64+
* Helper function for traversing "returns" (return statements or the
65+
* returned expression in the case of an arrow function) of a function
66+
*
67+
* @param {ASTNode} ASTNode The AST node being checked
68+
* @param {function} enterFunc Function to execute for each returnStatement found
69+
* @returns {undefined}
70+
*/
71+
function traverseReturns(ASTNode, enterFunc) {
72+
const nodeType = ASTNode.type;
73+
74+
if (nodeType === 'ReturnStatement') {
75+
return enterFunc(ASTNode);
76+
}
77+
78+
if (nodeType === 'ArrowFunctionExpression' && ASTNode.expression) {
79+
return enterFunc(ASTNode.body);
80+
}
81+
82+
if (nodeType !== 'FunctionExpression'
83+
&& nodeType !== 'FunctionDeclaration'
84+
&& nodeType !== 'ArrowFunctionExpression'
85+
&& nodeType !== 'MethodDefinition'
86+
) {
87+
throw new TypeError('only function nodes are expected');
88+
}
89+
90+
traverse(ASTNode.body, {
91+
enter(node) {
92+
switch (node.type) {
93+
case 'ReturnStatement':
94+
this.skip();
95+
return enterFunc(node);
96+
case 'FunctionExpression':
97+
case 'FunctionDeclaration':
98+
case 'ArrowFunctionExpression':
99+
return this.skip();
100+
default:
101+
}
102+
}
103+
});
104+
}
105+
40106
/**
41107
* Get node with property's name
42108
* @param {Object} node - Property.
@@ -268,6 +334,7 @@ function isTSTypeParameterInstantiation(node) {
268334
}
269335

270336
module.exports = {
337+
traverse,
271338
findReturnStatement,
272339
getFirstNodeInLine,
273340
getPropertyName,
@@ -280,6 +347,7 @@ module.exports = {
280347
isFunctionLikeExpression,
281348
isNodeFirstInLine,
282349
unwrapTSAsExpression,
350+
traverseReturns,
283351
isTSTypeReference,
284352
isTSTypeAnnotation,
285353
isTSTypeLiteral,

‎lib/util/jsx.js

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@
44

55
'use strict';
66

7+
const estraverse = require('estraverse');
78
const elementType = require('jsx-ast-utils/elementType');
89

10+
const astUtil = require('./ast');
11+
912
// See https://github.com/babel/babel/blob/ce420ba51c68591e057696ef43e028f41c6e04cd/packages/babel-types/src/validators/react/isCompatTag.js
1013
// for why we only test for the first character
1114
const COMPAT_TAG_REGEX = /^[a-z]/;
@@ -79,10 +82,75 @@ function isWhiteSpaces(value) {
7982
return typeof value === 'string' ? /^\s*$/.test(value) : false;
8083
}
8184

85+
/**
86+
* Check if the node is returning JSX or null
87+
*
88+
* @param {Function} isCreateElement Function to determine if a CallExpresion is
89+
* a createElement one
90+
* @param {ASTNode} ASTnode The AST node being checked
91+
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
92+
* @param {Boolean} [ignoreNull] If true, null return values will be ignored
93+
* @returns {Boolean} True if the node is returning JSX or null, false if not
94+
*/
95+
function isReturningJSX(isCreateElement, ASTnode, strict, ignoreNull) {
96+
let found = false;
97+
astUtil.traverseReturns(ASTnode, (node) => {
98+
// Traverse return statement
99+
astUtil.traverse(node, {
100+
enter(childNode) {
101+
const setFound = () => {
102+
found = true;
103+
this.skip();
104+
};
105+
switch (childNode.type) {
106+
case 'FunctionExpression':
107+
case 'FunctionDeclaration':
108+
case 'ArrowFunctionExpression':
109+
// Do not traverse into inner function definitions
110+
return this.skip();
111+
case 'ConditionalExpression':
112+
if (!strict) break;
113+
if (isJSX(childNode.consequent) && isJSX(childNode.alternate)) {
114+
setFound();
115+
}
116+
this.skip();
117+
break;
118+
case 'LogicalExpression':
119+
if (!strict) break;
120+
if (isJSX(childNode.left) && isJSX(childNode.right)) {
121+
setFound();
122+
}
123+
this.skip();
124+
break;
125+
case 'JSXElement':
126+
case 'JSXFragment':
127+
setFound(); break;
128+
case 'CallExpression':
129+
if (isCreateElement(childNode)) {
130+
setFound();
131+
}
132+
break;
133+
case 'Literal':
134+
if (!ignoreNull && childNode.value === null) {
135+
setFound();
136+
}
137+
break;
138+
default:
139+
}
140+
}
141+
});
142+
143+
return found && estraverse.VisitorOption.Break;
144+
});
145+
146+
return found;
147+
}
148+
82149
module.exports = {
83150
isDOMComponent,
84151
isFragment,
85152
isJSX,
86153
isJSXAttributeKey,
87-
isWhiteSpaces
154+
isWhiteSpaces,
155+
isReturningJSX
88156
};

‎package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
"array-includes": "^3.1.3",
3232
"array.prototype.flatmap": "^1.2.4",
3333
"doctrine": "^2.1.0",
34+
"estraverse": "^5.2.0",
3435
"has": "^1.0.3",
3536
"jsx-ast-utils": "^2.4.1 || ^3.0.0",
3637
"minimatch": "^3.0.4",

‎tests/lib/rules/destructuring-assignment.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,15 @@ ruleTester.run('destructuring-assignment', rule, {
187187
`,
188188
options: ['always'],
189189
parser: parsers.BABEL_ESLINT
190+
}, {
191+
code: `
192+
const obj = {
193+
foo(arg) {
194+
const a = arg.func();
195+
return null;
196+
},
197+
};
198+
`
190199
}],
191200

192201
invalid: [{

0 commit comments

Comments
 (0)