-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
detect used props in jsx #946
Conversation
@@ -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 |
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.
This comment should probably be updated.
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.
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'; |
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 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.
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.
Also, does this bug exist for folks not using JSX? (i.e. React.createElement
)
React.createElement("a", { onClick: function onClick() {
return props.previousPage();
} });
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.
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
fixes #885