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

[Fix] no-unused-prop-types: false positive with callback #2375

Merged
merged 1 commit into from Sep 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 50 additions & 33 deletions lib/util/Components.js
Expand Up @@ -577,50 +577,67 @@ function componentRule(rule, context) {
},

/**
* Get the parent stateless component node from the current scope
*
* @returns {ASTNode} component node, null if we are not in a component
* @param {ASTNode} node
* @returns {boolean}
*/
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;
isInAllowedPositionForComponent(node) {
switch (node.parent.type) {
case 'VariableDeclarator':
case 'AssignmentExpression':
case 'Property':
case 'ReturnStatement':
case 'ExportDefaultDeclaration': {
return true;
}
// Stop moving up if we reach a class or an argument (like a callback)
if (isClass || isArgument) {
return null;
case 'SequenceExpression': {
return utils.isInAllowedPositionForComponent(node.parent) &&
node === node.parent.expressions[node.parent.expressions.length - 1];
}
// 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)) {
default:
return false;
}
},

/**
* 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}
*/
getStatelessComponent(node) {
if (node.type === 'FunctionDeclaration') {
if (utils.isReturningJSXOrNull(node)) {
return node;
}
scope = scope.upper;
}
return null;

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

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

return undefined;
},

/**
* 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
* Get the parent stateless component node from the current scope
*
* @returns {ASTNode} component node, null if we are not in a component
*/
getArrowFunctionScope(scope) {
scope = scope.upper;
getParentStatelessComponent() {
let scope = context.getScope();
while (scope) {
if (astUtil.isFunction(scope.block) || astUtil.isClass(scope.block)) {
return scope;
const node = scope.block;
const statelessComponent = utils.getStatelessComponent(node);
if (statelessComponent) {
return statelessComponent;
}
scope = scope.upper;
}
Expand Down
10 changes: 0 additions & 10 deletions lib/util/ast.js
Expand Up @@ -133,15 +133,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 @@ -198,7 +189,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}]
ljharb marked this conversation as resolved.
Show resolved Hide resolved
}, {
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.'
}]
}
]
});