Skip to content

Commit

Permalink
[Fix] component detection: use estraverse to improve component dete…
Browse files Browse the repository at this point in the history 
…ction

Fixes #2960.
  • Loading branch information
@Wesitos @ljharb
Wesitos authored and ljharb committed May 24, 2021
1 parent 291acbf commit 9a8a4b5
Show file tree
Hide file tree
Showing 9 changed files with 351 additions and 90 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -5,6 +5,11 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

## Unreleased

### Fixed
* component detection: use `estraverse` to improve component detection ([#2992][] @Wesitos)

[#2992]: https://github.com/yannickcr/eslint-plugin-react/pull/2992

## [7.24.0] - 2021.05.27

### Added
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/no-typos.js
View file
Expand Up @@ -241,8 +241,9 @@ module.exports = {

if (
relatedComponent
&& (utils.isES6Component(relatedComponent.node) || utils.isReturningJSX(relatedComponent.node))
&& (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
&& (utils.isES6Component(relatedComponent.node) || (
relatedComponent.node.type !== 'ClassDeclaration' && utils.isReturningJSX(relatedComponent.node)))
&& (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
) {
reportErrorIfPropertyCasingTypo(node.parent.right, node.property, true);
}
Expand Down
102 changes: 15 additions & 87 deletions lib/util/Components.js
View file
Expand Up @@ -47,36 +47,6 @@ function mergeUsedPropTypes(propsList, newPropsList) {
return propsList.concat(propsToAdd);
}

function isReturnsConditionalJSX(node, property, strict) {
const returnsConditionalJSXConsequent = node[property]
&& node[property].type === 'ConditionalExpression'
&& jsxUtil.isJSX(node[property].consequent);
const returnsConditionalJSXAlternate = node[property]
&& node[property].type === 'ConditionalExpression'
&& jsxUtil.isJSX(node[property].alternate);
return strict
? (returnsConditionalJSXConsequent && returnsConditionalJSXAlternate)
: (returnsConditionalJSXConsequent || returnsConditionalJSXAlternate);
}

function isReturnsLogicalJSX(node, property, strict) {
const returnsLogicalJSXLeft = node[property]
&& node[property].type === 'LogicalExpression'
&& jsxUtil.isJSX(node[property].left);
const returnsLogicalJSXRight = node[property]
&& node[property].type === 'LogicalExpression'
&& jsxUtil.isJSX(node[property].right);
return strict
? (returnsLogicalJSXLeft && returnsLogicalJSXRight)
: (returnsLogicalJSXLeft || returnsLogicalJSXRight);
}

function isReturnsSequentialJSX(node, property) {
return node[property]
&& node[property].type === 'SequenceExpression'
&& jsxUtil.isJSX(node[property].expressions[node[property].expressions.length - 1]);
}

const Lists = new WeakMap();

/**
Expand Down Expand Up @@ -466,65 +436,12 @@ function componentRule(rule, context) {
};
},

/**
* Check if the node is returning JSX
*
* @param {ASTNode} ASTnode The AST node being checked
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
* @returns {Boolean} True if the node is returning JSX, false if not
*/
isReturningJSX(ASTnode, strict) {
const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
const node = nodeAndProperty.node;
const property = nodeAndProperty.property;

if (!node) {
return false;
}

const returnsConditionalJSX = isReturnsConditionalJSX(node, property, strict);
const returnsLogicalJSX = isReturnsLogicalJSX(node, property, strict);
const returnsSequentialJSX = isReturnsSequentialJSX(node, property);

const returnsJSX = node[property] && jsxUtil.isJSX(node[property]);
const returnsPragmaCreateElement = this.isCreateElement(node[property]);

return !!(
returnsConditionalJSX
|| returnsLogicalJSX
|| returnsSequentialJSX
|| returnsJSX
|| returnsPragmaCreateElement
);
isReturningJSX(ASTNode, strict) {
return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict, true);
},

/**
* Check if the node is returning null
*
* @param {ASTNode} ASTnode The AST node being checked
* @returns {Boolean} True if the node is returning null, false if not
*/
isReturningNull(ASTnode) {
const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
const property = nodeAndProperty.property;
const node = nodeAndProperty.node;

if (!node) {
return false;
}

return node[property] && node[property].value === null;
},

/**
* Check if the node is returning JSX or null
*
* @param {ASTNode} ASTNode The AST node being checked
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
* @returns {Boolean} True if the node is returning JSX or null, false if not
*/
isReturningJSXOrNull(ASTNode, strict) {
return utils.isReturningJSX(ASTNode, strict) || utils.isReturningNull(ASTNode);
return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict);
},

getPragmaComponentWrapper(node) {
Expand Down Expand Up @@ -734,9 +651,20 @@ function componentRule(rule, context) {
return undefined;
}
if (utils.isInAllowedPositionForComponent(node) && utils.isReturningJSXOrNull(node)) {
if (!node.id || isFirstLetterCapitalized(node.id.name)) {
const isMethod = node.parent.type === 'Property' && node.parent.method;

if (isMethod && !isFirstLetterCapitalized(node.parent.key.name)) {
return utils.isReturningJSX(node) ? node : undefined;
}

if (node.id && isFirstLetterCapitalized(node.id.name)) {
return node;
}

if (!node.id) {
return node;
}

return undefined;
}

Expand Down
68 changes: 68 additions & 0 deletions lib/util/ast.js
View file
Expand Up @@ -4,6 +4,29 @@

'use strict';

const estraverse = require('estraverse');

/**
* Wrapper for estraverse.traverse
*
* @param {ASTNode} ASTnode The AST node being checked
* @param {Object} visitor Visitor Object for estraverse
*/
function traverse(ASTnode, visitor) {
const opts = Object.assign({}, {
fallback(node) {
return Object.keys(node).filter((key) => key === 'children' || key === 'argument');
}
}, visitor);

opts.keys = Object.assign({}, visitor.keys, {
JSXElement: ['children'],
JSXFragment: ['children']
});

estraverse.traverse(ASTnode, opts);
}

/**
* Find a return statment in the current node
*
Expand Down Expand Up @@ -37,6 +60,49 @@ function findReturnStatement(node) {
}(bodyNodes));
}

/**
* Helper function for traversing "returns" (return statements or the
* returned expression in the case of an arrow function) of a function
*
* @param {ASTNode} ASTNode The AST node being checked
* @param {function} enterFunc Function to execute for each returnStatement found
* @returns {undefined}
*/
function traverseReturns(ASTNode, enterFunc) {
const nodeType = ASTNode.type;

if (nodeType === 'ReturnStatement') {
return enterFunc(ASTNode);
}

if (nodeType === 'ArrowFunctionExpression' && ASTNode.expression) {
return enterFunc(ASTNode.body);
}

if (nodeType !== 'FunctionExpression'
&& nodeType !== 'FunctionDeclaration'
&& nodeType !== 'ArrowFunctionExpression'
&& nodeType !== 'MethodDefinition'
) {
throw new TypeError('only function nodes are expected');
}

traverse(ASTNode.body, {
enter(node) {
switch (node.type) {
case 'ReturnStatement':
this.skip();
return enterFunc(node);
case 'FunctionExpression':
case 'FunctionDeclaration':
case 'ArrowFunctionExpression':
return this.skip();
default:
}
}
});
}

/**
* Get node with property's name
* @param {Object} node - Property.
Expand Down Expand Up @@ -268,6 +334,7 @@ function isTSTypeParameterInstantiation(node) {
}

module.exports = {
traverse,
findReturnStatement,
getFirstNodeInLine,
getPropertyName,
Expand All @@ -280,6 +347,7 @@ module.exports = {
isFunctionLikeExpression,
isNodeFirstInLine,
unwrapTSAsExpression,
traverseReturns,
isTSTypeReference,
isTSTypeAnnotation,
isTSTypeLiteral,
Expand Down
70 changes: 69 additions & 1 deletion lib/util/jsx.js
View file
Expand Up @@ -4,8 +4,11 @@

'use strict';

const estraverse = require('estraverse');
const elementType = require('jsx-ast-utils/elementType');

const astUtil = require('./ast');

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

/**
* Check if the node is returning JSX or null
*
* @param {Function} isCreateElement Function to determine if a CallExpresion is
* a createElement one
* @param {ASTNode} ASTnode The AST node being checked
* @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
* @param {Boolean} [ignoreNull] If true, null return values will be ignored
* @returns {Boolean} True if the node is returning JSX or null, false if not
*/
function isReturningJSX(isCreateElement, ASTnode, strict, ignoreNull) {
let found = false;
astUtil.traverseReturns(ASTnode, (node) => {
// Traverse return statement
astUtil.traverse(node, {
enter(childNode) {
const setFound = () => {
found = true;
this.skip();
};
switch (childNode.type) {
case 'FunctionExpression':
case 'FunctionDeclaration':
case 'ArrowFunctionExpression':
// Do not traverse into inner function definitions
return this.skip();
case 'ConditionalExpression':
if (!strict) break;
if (isJSX(childNode.consequent) && isJSX(childNode.alternate)) {
setFound();
}
this.skip();
break;
case 'LogicalExpression':
if (!strict) break;
if (isJSX(childNode.left) && isJSX(childNode.right)) {
setFound();
}
this.skip();
break;
case 'JSXElement':
case 'JSXFragment':
setFound(); break;
case 'CallExpression':
if (isCreateElement(childNode)) {
setFound();
}
break;
case 'Literal':
if (!ignoreNull && childNode.value === null) {
setFound();
}
break;
default:
}
}
});

return found && estraverse.VisitorOption.Break;
});

return found;
}

module.exports = {
isDOMComponent,
isFragment,
isJSX,
isJSXAttributeKey,
isWhiteSpaces
isWhiteSpaces,
isReturningJSX
};
1 change: 1 addition & 0 deletions package.json
View file
Expand Up @@ -31,6 +31,7 @@
"array-includes": "^3.1.3",
"array.prototype.flatmap": "^1.2.4",
"doctrine": "^2.1.0",
"estraverse": "^5.2.0",
"has": "^1.0.3",
"jsx-ast-utils": "^2.4.1 || ^3.0.0",
"minimatch": "^3.0.4",
Expand Down
9 changes: 9 additions & 0 deletions tests/lib/rules/destructuring-assignment.js
View file
Expand Up @@ -187,6 +187,15 @@ ruleTester.run('destructuring-assignment', rule, {
`,
options: ['always'],
parser: parsers.BABEL_ESLINT
}, {
code: `
const obj = {
foo(arg) {
const a = arg.func();
return null;
},
};
`
}],

invalid: [{
Expand Down

0 comments on commit 9a8a4b5

Please sign in to comment.