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

Improve component detection #2992

Merged
merged 1 commit into from May 30, 2021
Merged

Conversation

Wesitos
Copy link
Contributor

@Wesitos Wesitos commented May 25, 2021

Copy link
Member

@ljharb ljharb left a 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 Show resolved Hide resolved
lib/util/ast.js Outdated
Comment on lines 79 to 88
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;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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');
}

Copy link
Contributor Author

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-commenter
Copy link

codecov-commenter commented May 29, 2021

Codecov Report

Merging #2992 (306132b) into master (6c1e5b3) will decrease coverage by 0.37%.
The diff coverage is 91.46%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
lib/util/jsx.js 92.64% <87.80%> (-7.36%) ⬇️
lib/util/ast.js 94.83% <93.10%> (-3.58%) ⬇️
lib/rules/no-typos.js 100.00% <100.00%> (ø)
lib/util/Components.js 95.28% <100.00%> (-3.58%) ⬇️
lib/util/version.js 97.72% <0.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c1e5b3...306132b. Read the comment docs.

@Wesitos Wesitos marked this pull request as draft May 29, 2021 23:47
@Wesitos Wesitos marked this pull request as ready for review May 30, 2021 01:20
@Wesitos Wesitos requested a review from ljharb May 30, 2021 01:20
Copy link
Member

@ljharb ljharb left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

destructuring-assignment shouldn't trigger on method calls
3 participants