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

detect used props in jsx #946

Merged
merged 2 commits into from
Feb 15, 2017
Merged

detect used props in jsx #946

merged 2 commits into from
Feb 15, 2017

Conversation

BarryThePenguin
Copy link
Contributor

fixes #885

@@ -347,12 +347,13 @@ function componentRule(rule, context) {
var isFunction = /Function/.test(node.type); // Functions
var isMethod = node.parent && node.parent.type === 'MethodDefinition'; // Classes methods
var isArgument = node.parent && node.parent.type === 'CallExpression'; // Arguments (callback, etc.)
var isJSX = node.parent && node.parent.type === 'JSXExpressionContainer';
// Stop moving up if we reach a class or an argument (like a callback)
if (isClass || isArgument) {
return null;
}
// Return the node if it is a function that is not a class method
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment should probably be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -347,12 +347,13 @@ function componentRule(rule, context) {
var isFunction = /Function/.test(node.type); // Functions
var isMethod = node.parent && node.parent.type === 'MethodDefinition'; // Classes methods
var isArgument = node.parent && node.parent.type === 'CallExpression'; // Arguments (callback, etc.)
var isJSX = node.parent && node.parent.type === 'JSXExpressionContainer';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be misunderstanding what this is checking for here, but I'm not sure this variable name makes sense. I think isJSXExpressionContainerExpression would be more accurate.

Additionally, it might be nice to include a comment with an example of what type of code this identifies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, does this bug exist for folks not using JSX? (i.e. React.createElement)

React.createElement("a", { onClick: function onClick() {
    return props.previousPage();
  } });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that functions inside a JSXExpressionContainer are bing mistaken for stateless components.

For example: <button onClick={() => props.handleClick()}></button>
where {() => props.handleClick()} is theJSXExpressionContainer

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

Successfully merging this pull request may close these issues.

no-unused-prop-types sometimes fails to detect props in functions on stateless components
2 participants