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

[Refactor]: improve performance for detecting function components #3265

Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -19,7 +19,9 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
* [readme] remove global usage and eslint version from readme ([#3254][] @aladdin-add)
* [Refactor] fix linter errors ([#3261][] @golopot)
* [Docs] [`no-unused-prop-types`]: fix syntax errors ([#3259][] @mrdulin)
* [Refactor] improve performance for detecting function components ([#3265][] @golopot)

[#3265]: https://github.com/yannickcr/eslint-plugin-react/pull/3265
[#3261]: https://github.com/yannickcr/eslint-plugin-react/pull/3261
[#3260]: https://github.com/yannickcr/eslint-plugin-react/pull/3260
[#3259]: https://github.com/yannickcr/eslint-plugin-react/pull/3259
Expand Down
30 changes: 21 additions & 9 deletions lib/util/ast.js
Expand Up @@ -62,24 +62,28 @@ function findReturnStatement(node) {
return loopNodes(bodyNodes);
}

// eslint-disable-next-line valid-jsdoc -- valid-jsdoc cannot parse function types.
/**
* 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 {Context} context The context of `ASTNode`.
* @param {function} enterFunc Function to execute for each returnStatement found
* @param {(returnValue: ASTNode, breakTraverse: () => void) => void} onReturn
* Function to execute for each returnStatement found
* @returns {undefined}
*/
function traverseReturns(ASTNode, context, enterFunc) {
function traverseReturns(ASTNode, context, onReturn) {
const nodeType = ASTNode.type;

if (nodeType === 'ReturnStatement') {
return enterFunc(ASTNode);
onReturn(ASTNode.argument, () => {});
return;
}

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

/* TODO: properly warn on React.forwardRefs having typo properties
Expand Down Expand Up @@ -111,15 +115,23 @@ function traverseReturns(ASTNode, context, enterFunc) {

traverse(ASTNode.body, {
enter(node) {
const breakTraverse = () => {
this.break();
};
switch (node.type) {
case 'ReturnStatement':
this.skip();
return enterFunc(node);
case 'FunctionExpression':
case 'FunctionDeclaration':
case 'ArrowFunctionExpression':
return this.skip();
onReturn(node.argument, breakTraverse);
return;
case 'BlockStatement':
case 'IfStatement':
case 'ForStatement':
case 'WhileStatement':
case 'SwitchStatement':
case 'SwitchCase':
return;
default:
this.skip();
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoids unnecessary traversals by skipping all node types but the handled cases.

});
Expand Down
96 changes: 40 additions & 56 deletions lib/util/jsx.js
Expand Up @@ -4,7 +4,6 @@

'use strict';

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

const astUtil = require('./ast');
Expand Down Expand Up @@ -95,63 +94,48 @@ function isWhiteSpaces(value) {
* @returns {Boolean} True if the node is returning JSX or null, false if not
*/
function isReturningJSX(isCreateElement, ASTnode, context, strict, ignoreNull) {
let found = false;
astUtil.traverseReturns(ASTnode, context, (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();
}
this.skip();
break;
case 'Literal':
if (!ignoreNull && childNode.value === null) {
setFound();
}
break;
case 'Identifier': {
const variable = variableUtil.findVariableByName(context, childNode.name);
if (isJSX(variable)) {
setFound();
}
break;
}
default:
const isJSXValue = (node) => {
if (!node) {
return false;
}
switch (node.type) {
case 'ConditionalExpression':
if (strict) {
return isJSXValue(node.consequent) && isJSXValue(node.alternate);
}
},
});
return isJSXValue(node.consequent) || isJSXValue(node.alternate);
case 'LogicalExpression':
if (strict) {
return isJSXValue(node.left) && isJSXValue(node.right);
}
return isJSXValue(node.left) || isJSXValue(node.right);
case 'SequenceExpression':
return isJSXValue(node.expressions[node.expressions.length - 1]);
case 'JSXElement':
case 'JSXFragment':
return true;
case 'CallExpression':
return isCreateElement(node);
case 'Literal':
if (!ignoreNull && node.value === null) {
return true;
}
return false;
case 'Identifier': {
const variable = variableUtil.findVariableByName(context, node.name);
return isJSX(variable);
}
default:
return false;
}
};

return found && estraverse.VisitorOption.Break;
let found = false;
astUtil.traverseReturns(ASTnode, context, (node, breakTraverse) => {
if (isJSXValue(node)) {
found = true;
breakTraverse();
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above and simplify by not using astUtil.traverse.


return found;
Expand Down
4 changes: 2 additions & 2 deletions tests/util/ast.js
Expand Up @@ -96,8 +96,8 @@ describe('ast', () => {

assert.strictEqual(enterCalls.length, 6);

enterCalls.forEach((node, idx) => {
assert.strictEqual(node.lastArg.argument.value, idx);
enterCalls.forEach((call, idx) => {
assert.strictEqual(call.args[0].value, idx);
ljharb marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
Expand Down