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
Improve component detection #2992
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, hopefully estraverse runs in all the eslint and node versions we support.
lib/util/ast.js
Outdated
switch (ASTnode.type) { | ||
case 'FunctionExpression': | ||
case 'FunctionDeclaration': | ||
case 'MethodDefinition': | ||
break; | ||
case 'ReturnStatement': | ||
enterFunc(ASTnode); | ||
return; | ||
case 'ArrowFunctionExpression': | ||
if (ASTnode.expression) { | ||
enterFunc(ASTnode.body); | ||
return; | ||
} | ||
break; | ||
default: | ||
// We expect only to receive function nodes | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (ASTnode.type) { | |
case 'FunctionExpression': | |
case 'FunctionDeclaration': | |
case 'MethodDefinition': | |
break; | |
case 'ReturnStatement': | |
enterFunc(ASTnode); | |
return; | |
case 'ArrowFunctionExpression': | |
if (ASTnode.expression) { | |
enterFunc(ASTnode.body); | |
return; | |
} | |
break; | |
default: | |
// We expect only to receive function nodes | |
return; | |
} | |
if (ASTNode.type === 'ReturnStatement') { | |
enterFunc(ASTnode); | |
} else if (ASTNode.type === 'ArrowFunctionExpression') { | |
if (ASTNode.expression) { | |
enterFunc(ASTnode.body); | |
} | |
} else if (ASTNode.type === 'ReturnStatement') { | |
enterFunc(ASTnode); | |
} else if (ASTNode.type !== 'FunctionExpression' && ASTNode.type !== 'FunctionDeclaration' && ASTNode.type !== 'MethodDefinition') { | |
throw new TypeError('only function nodes are expected'); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this with "If-return"s. I think it's a bit cleaner than a chain of 'else if".
isReturningJSX
(lib/util/Components.js) sometimes is called with ClassDeclaration
nodes. Now only a few no-typos
tests were broken by this (I fixed it). IMHO, doing nothing here is safer than throwing an exception.
Codecov Report
@@ Coverage Diff @@
## master #2992 +/- ##
==========================================
- Coverage 97.59% 97.21% -0.38%
==========================================
Files 110 110
Lines 7268 7297 +29
Branches 2651 2657 +6
==========================================
+ Hits 7093 7094 +1
- Misses 175 203 +28
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the code deletion; I'm nervous about the delegation of responsibility; I like farming responsibility out to a dep; that all existing tests pass makes me confident.
306132b
to
9a8a4b5
Compare
traverseReturns
usingestraverse
to improve return value analysis.isReturningJSX
function to handle return statements nested inside other structures (IfConditions
,SwitchStatements
, etc)destructuring-assignment
shouldn't trigger on method calls #2960