Skip to content

Commit

Permalink
[Fix] no-unused-prop-types: false positive with callback
Browse files Browse the repository at this point in the history
  • Loading branch information
golopot committed Sep 3, 2019
1 parent fcfee49 commit 73bb36d
Show file tree
Hide file tree
Showing 8 changed files with 135 additions and 51 deletions.
83 changes: 50 additions & 33 deletions lib/util/Components.js
Expand Up @@ -581,45 +581,62 @@ function componentRule(rule, context) {
* @returns {ASTNode} component node, null if we are not in a component
*/
getParentStatelessComponent() {
let scope = context.getScope();
while (scope) {
const node = scope.block;
const isFunction = /Function/.test(node.type); // Functions
const isArrowFunction = astUtil.isArrowFunction(node);
const enclosingScope = isArrowFunction ? utils.getArrowFunctionScope(scope) : scope;
const enclosingScopeParent = enclosingScope && enclosingScope.block.parent;
const isClass = enclosingScope && astUtil.isClass(enclosingScope.block);
const isMethod = enclosingScopeParent && enclosingScopeParent.type === 'MethodDefinition'; // Classes methods
const isArgument = node.parent && node.parent.type === 'CallExpression'; // Arguments (callback, etc.)
// Attribute Expressions inside JSX Elements (<button onClick={() => props.handleClick()}></button>)
const isJSXExpressionContainer = node.parent && node.parent.type === 'JSXExpressionContainer';
const pragmaComponentWrapper = this.getPragmaComponentWrapper(node);
if (isFunction && pragmaComponentWrapper) {
return pragmaComponentWrapper;
/**
* @param {ASTNode} node
* @returns {boolean}
*/
function isInAllowedPositiionForComponent(node) {
switch (node.parent.type) {
case 'VariableDeclarator':
case 'AssignmentExpression':
case 'Property':
case 'ReturnStatement':
case 'ExportDefaultDeclaration': {
return true;
}
case 'SequenceExpression': {
return isInAllowedPositiionForComponent(node.parent) &&
node === node.parent.expressions[node.parent.expressions.length - 1];
}
default:
return false;
}
// Stop moving up if we reach a class or an argument (like a callback)
if (isClass || isArgument) {
return null;
}

/**
* Get node if node is a stateless component, or node.parent in cases like
* `React.memo` or `React.forwardRef`. Otherwise returns `undefined`.
* @param {ASTNode} node
* @returns {ASTNode | undefined}
*/
function getStatelessComponent(node) {
if (node.type === 'FunctionDeclaration') {
if (utils.isReturningJSXOrNull(node)) {
return node;
}
}
// Return the node if it is a function that is not a class method and is not inside a JSX Element
if (isFunction && !isMethod && !isJSXExpressionContainer && utils.isReturningJSXOrNull(node)) {
return node;

if (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') {
if (isInAllowedPositiionForComponent(node) && utils.isReturningJSXOrNull(node)) {
return node;
}

// Case like `React.memo(() => <></>)` or `React.forwardRef(...)`
const pragmaComponentWrapper = utils.getPragmaComponentWrapper(node);
if (pragmaComponentWrapper) {
return pragmaComponentWrapper;
}
}
scope = scope.upper;

return undefined;
}
return null;
},

/**
* Get an enclosing scope used to find `this` value by an arrow function
* @param {Scope} scope Current scope
* @returns {Scope} An enclosing scope used by an arrow function
*/
getArrowFunctionScope(scope) {
scope = scope.upper;
let scope = context.getScope();
while (scope) {
if (astUtil.isFunction(scope.block) || astUtil.isClass(scope.block)) {
return scope;
const node = scope.block;
const statelessComponent = getStatelessComponent(node);
if (statelessComponent) {
return statelessComponent;
}
scope = scope.upper;
}
Expand Down
10 changes: 0 additions & 10 deletions lib/util/ast.js
Expand Up @@ -132,15 +132,6 @@ function isFunction(node) {
return node.type === 'FunctionExpression' || node.type === 'FunctionDeclaration';
}

/**
* Checks if the node is an arrow function.
* @param {ASTNode} node The node to check
* @return {Boolean} true if it's an arrow function
*/
function isArrowFunction(node) {
return node.type === 'ArrowFunctionExpression';
}

/**
* Checks if the node is a class.
* @param {ASTNode} node The node to check
Expand Down Expand Up @@ -196,7 +187,6 @@ module.exports = {
getPropertyNameNode,
getComponentProperties,
getKeyValue,
isArrowFunction,
isAssignmentLHS,
isClass,
isFunction,
Expand Down
23 changes: 23 additions & 0 deletions tests/lib/rules/destructuring-assignment.js
Expand Up @@ -256,6 +256,29 @@ ruleTester.run('destructuring-assignment', rule, {
errors: [
{message: 'Must use destructuring props assignment'}
]
}, {
code: `
module.exports = {
Foo(props) {
return <p>{props.a}</p>;
}
}
`,
errors: [{message: 'Must use destructuring props assignment'}]
}, {
code: `
export default function Foo(props) {
return <p>{props.a}</p>;
}
`,
errors: [{message: 'Must use destructuring props assignment'}]
}, {
code: `
function hof() {
return (props) => <p>{props.a}</p>;
}
`,
errors: [{message: 'Must use destructuring props assignment'}]
}, {
code: `const Foo = class extends React.PureComponent {
render() {
Expand Down
12 changes: 12 additions & 0 deletions tests/lib/rules/display-name.js
Expand Up @@ -661,6 +661,18 @@ ruleTester.run('display-name', rule, {
errors: [{
message: 'Component definition is missing display name'
}]
}, {
code: `
function Hof() {
return function () {
return <div />
}
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: 'Component definition is missing display name'
}]
}, {
code: `
import React, { createElement } from "react";
Expand Down
21 changes: 20 additions & 1 deletion tests/lib/rules/no-multi-comp.js
Expand Up @@ -312,7 +312,26 @@ ruleTester.run('no-multi-comp', rule, {
message: 'Declare only one React component per file',
line: 6
}]
}, {
},
{
code: `
exports.Foo = function Foo() {
return <></>
}
exports.createSomeComponent = function createSomeComponent(opts) {
return function Foo() {
return <>{opts.a}</>
}
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: 'Declare only one React component per file',
line: 7
}]
},
{
code: `
class StoreListItem extends React.PureComponent {
// A bunch of stuff here
Expand Down
7 changes: 0 additions & 7 deletions tests/lib/rules/no-this-in-sfc.js
Expand Up @@ -217,13 +217,6 @@ ruleTester.run('no-this-in-sfc', rule, {
return <div onClick={onClick}>{this.props.foo}</div>;
}`,
errors: [{message: ERROR_MESSAGE}, {message: ERROR_MESSAGE}]
}, {
code: `
() => {
this.something();
return null;
}`,
errors: [{message: ERROR_MESSAGE}]
}, {
code: `
class Foo {
Expand Down
16 changes: 16 additions & 0 deletions tests/lib/rules/no-unused-prop-types.js
Expand Up @@ -1856,6 +1856,22 @@ ruleTester.run('no-unused-prop-types', rule, {
' previousPage: PropTypes.func.isRequired,',
'};'
].join('\n')
}, {
// issue 2350
code: `
function Foo(props) {
useEffect(() => {
const { a } = props;
document.title = a;
});
return <p/>;
}
Foo.propTypes = {
a: PropTypes.string,
}
`
}, {
code: [
'class Hello extends Component {',
Expand Down
14 changes: 14 additions & 0 deletions tests/lib/rules/require-default-props.js
Expand Up @@ -2225,6 +2225,20 @@ ruleTester.run('require-default-props', rule, {
errors: [{
message: 'propType "usedProp" is not required, but has no corresponding defaultProps declaration.'
}]
},
{
code: `
Foo.propTypes = {
a: PropTypes.string,
}
export default function Foo(props) {
return <p>{props.a}<p/>
};
`,
errors: [{
message: 'propType "a" is not required, but has no corresponding defaultProps declaration.'
}]
}
]
});

0 comments on commit 73bb36d

Please sign in to comment.