Skip to content

Extract duplicate code between prop-types and no-unused-prop-types #1 #1536

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Extract duplicate code between prop-types and no-unused-prop-types #1 #1536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6907ae6
Replace inComponentWillReceiveProps and inConstructor with inLifecycl…
jseminck Nov 14, 2017
e9862d1
Replace existing isInLifecycleMethod with inLifecycleMethod. Now prop…
jseminck Nov 14, 2017
13222de
Move inLifecycleMethod to astUtil
jseminck Nov 14, 2017
db0d571
Remove inConstructor
jseminck Nov 14, 2017
2fcbfbe
Add a test case for shouldComponentUpdate with nextProps in prop-types
jseminck Nov 14, 2017
14742f9
Re-add inConstructor
Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 15 additions & 78 deletions lib/rules/no-unused-prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const variable = require('../util/variable');
const annotations = require('../util/annotations');
const versionUtil = require('../util/version');
const propsUtil = require('../util/props');
const astUtil = require('../util/ast');

// ------------------------------------------------------------------------------
// Constants
Expand All @@ -21,7 +22,6 @@ const propsUtil = require('../util/props');
const DIRECT_PROPS_REGEX = /^props\s*(\.|\[)/;
const DIRECT_NEXT_PROPS_REGEX = /^nextProps\s*(\.|\[)/;
const DIRECT_PREV_PROPS_REGEX = /^prevProps\s*(\.|\[)/;
const LIFE_CYCLE_METHODS = ['componentWillReceiveProps', 'shouldComponentUpdate', 'componentWillUpdate', 'componentDidUpdate'];

// ------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -82,24 +82,6 @@ module.exports = {
return value;
}

/**
* Check if we are in a lifecycle method
* @return {boolean} true if we are in a class constructor, false if not
**/
function inLifeCycleMethod() {
let scope = context.getScope();
while (scope) {
if (
scope.block && scope.block.parent &&
scope.block.parent.key &&
LIFE_CYCLE_METHODS.indexOf(scope.block.parent.key.name) >= 0
) {
return true;
}
scope = scope.upper;
}
return false;
}

/**
* Check if the current node is in a setState updater method
Expand Down Expand Up @@ -155,7 +137,7 @@ module.exports = {
|| isPropArgumentInSetStateUpdater(node))
);
const isStatelessFunctionUsage = node.object.name === 'props';
return isClassUsage || isStatelessFunctionUsage || inLifeCycleMethod();
return isClassUsage || isStatelessFunctionUsage || astUtil.inLifeCycleMethod(context);
}

/**
Expand Down Expand Up @@ -241,40 +223,6 @@ module.exports = {
);
}

/**
* Returns true if the given node is a React Component lifecycle method
* @param {ASTNode} node The AST node being checked.
* @return {Boolean} True if the node is a lifecycle method
*/
function isNodeALifeCycleMethod(node) {
const nodeKeyName = (node.key || {}).name;
return (
node.kind === 'constructor' ||
nodeKeyName === 'componentWillReceiveProps' ||
nodeKeyName === 'shouldComponentUpdate' ||
nodeKeyName === 'componentWillUpdate' ||
nodeKeyName === 'componentDidUpdate'
);
}

/**
* Returns true if the given node is inside a React Component lifecycle
* method.
* @param {ASTNode} node The AST node being checked.
* @return {Boolean} True if the node is inside a lifecycle method
*/
function isInLifeCycleMethod(node) {
if (node.type === 'MethodDefinition' && isNodeALifeCycleMethod(node)) {
return true;
}

if (node.parent) {
return isInLifeCycleMethod(node.parent);
}

return false;
}

/**
* Checks if a prop init name matches common naming patterns
* @param {ASTNode} node The AST node being checked.
Expand Down Expand Up @@ -553,21 +501,6 @@ module.exports = {
}
}

/**
* Check if we are in a class constructor
* @return {boolean} true if we are in a class constructor, false if not
*/
function inConstructor() {
let scope = context.getScope();
while (scope) {
if (scope.block && scope.block.parent && scope.block.parent.kind === 'constructor') {
return true;
}
scope = scope.upper;
}
return false;
}

/**
* Retrieve the name of a property node
* @param {ASTNode} node The AST node with the property.
Expand All @@ -579,10 +512,11 @@ module.exports = {
const isDirectPrevProp = DIRECT_PREV_PROPS_REGEX.test(sourceCode.getText(node));
const isDirectSetStateProp = isPropArgumentInSetStateUpdater(node);
const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component();
const isNotInConstructor = !inConstructor(node);
const isNotInLifeCycleMethod = !inLifeCycleMethod();
const isNotInLifeCycleMethod = !astUtil.inLifeCycleMethod(context);
const isNotInConstructor = !astUtil.inConstructor(context);
const isNotInSetStateUpdater = !inSetStateUpdater();
if ((isDirectProp || isDirectNextProp || isDirectPrevProp || isDirectSetStateProp)
if (
(isDirectProp || isDirectNextProp || isDirectPrevProp || isDirectSetStateProp)
&& isInClassComponent
&& isNotInConstructor
&& isNotInLifeCycleMethod
Expand Down Expand Up @@ -668,10 +602,12 @@ module.exports = {
)
);
// let {firstname} = props
const genericDestructuring = isPropAttributeName(node) && (
utils.getParentStatelessComponent() ||
isInLifeCycleMethod(node)
);
const genericDestructuring =
isPropAttributeName(node) && (
utils.getParentStatelessComponent() ||
astUtil.inConstructor(context) ||
Copy link
Member

Choose a reason for hiding this comment

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

which test case exposes this missing inConstructor check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the old isInLifeCycleMethod function already was checking whether we are in constructor: node.kind === 'constructor'. Now it is split into inConstructor and inLifecycleMethod so the code is the same between this rule and prop-types.

In this case, the following two tests will fail when removing this line:

astUtil.inLifeCycleMethod(context)
);

if (thisDestructuring) {
properties = node.id.properties[i].value.properties;
Expand Down Expand Up @@ -992,7 +928,8 @@ module.exports = {
// let {firstname} = props
const statelessDestructuring = destructuring && isPropAttributeName(node) && (
utils.getParentStatelessComponent() ||
isInLifeCycleMethod(node)
astUtil.inConstructor(context) ||
Copy link
Member

Choose a reason for hiding this comment

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

also here, which test case exposes the need for this check?

Copy link
Contributor Author

@jseminck jseminck Nov 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Same as above: Note that the old isInLifeCycleMethod function already was checking whether we are in constructor: node.kind === 'constructor'. Now it is split into inConstructor and inLifecycleMethod so the code is the same between this rule and prop-types.

However, this case seems to be untested. 😞 In master we can remove this whole block and the tests still pass.

&& ( utils.getParentStatelessComponent() || isInLifeCycleMethod(node))

astUtil.inLifeCycleMethod(context)
);

if (!thisDestructuring && !statelessDestructuring) {
Expand Down Expand Up @@ -1058,7 +995,7 @@ module.exports = {
ObjectPattern: function(node) {
// If the object pattern is a destructured props object in a lifecycle
// method -- mark it for used props.
if (isNodeALifeCycleMethod(node.parent.parent)) {
if (astUtil.inConstructor(context) || astUtil.inLifeCycleMethod(context)) {
Copy link
Member

Choose a reason for hiding this comment

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

also here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Fails when removing inConstructor:

When removing the inLifecycleMethod then 5 tests fail (2 for componentWillReceiveProps, 1 for shouldComponentUpdate and 2 for componentDidUpdate)

I suppose that we can also test https://reactjs.org/docs/react-component.html#componentwillupdate which also has nextProps as the first argument.

node.properties.forEach((property, i) => {
if (i === 0) {
markPropTypesAsUsed(node.parent);
Expand Down
59 changes: 17 additions & 42 deletions lib/rules/prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const variable = require('../util/variable');
const annotations = require('../util/annotations');
const versionUtil = require('../util/version');
const propsUtil = require('../util/props');
const astUtil = require('../util/ast');

// ------------------------------------------------------------------------------
// Constants
Expand Down Expand Up @@ -86,39 +87,6 @@ module.exports = {
return value;
}

/**
* Check if we are in a class constructor
* @return {boolean} true if we are in a class constructor, false if not
*/
function inConstructor() {
let scope = context.getScope();
while (scope) {
if (scope.block && scope.block.parent && scope.block.parent.kind === 'constructor') {
return true;
}
scope = scope.upper;
}
return false;
}

/**
* Check if we are in a class constructor
* @return {boolean} true if we are in a class constructor, false if not
*/
function inComponentWillReceiveProps() {
let scope = context.getScope();
while (scope) {
if (
scope.block && scope.block.parent &&
scope.block.parent.key && scope.block.parent.key.name === 'componentWillReceiveProps'
) {
return true;
}
scope = scope.upper;
}
return false;
}

/**
* Checks if a prop is being assigned a value props.bar = 'bar'
* @param {ASTNode} node The AST node being checked.
Expand All @@ -144,7 +112,7 @@ module.exports = {
node.object.type === 'ThisExpression' && node.property.name === 'props'
);
const isStatelessFunctionUsage = node.object.name === 'props' && !isAssignmentToProp(node);
const isNextPropsUsage = node.object.name === 'nextProps' && inComponentWillReceiveProps();
const isNextPropsUsage = node.object.name === 'nextProps' && astUtil.inLifeCycleMethod(context);
return isClassUsage || isStatelessFunctionUsage || isNextPropsUsage;
}

Expand Down Expand Up @@ -534,9 +502,14 @@ module.exports = {
function getPropertyName(node) {
const isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node));
const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component();
const isNotInConstructor = !inConstructor();
const isNotInComponentWillReceiveProps = !inComponentWillReceiveProps();
if (isDirectProp && isInClassComponent && isNotInConstructor && isNotInComponentWillReceiveProps) {
const isNotInConstructor = !astUtil.inConstructor(context);
const isNotInLifecycleMethod = !astUtil.inLifeCycleMethod(context);
if (
isDirectProp &&
isInClassComponent &&
isNotInConstructor &&
isNotInLifecycleMethod
) {
return void 0;
}
if (!isDirectProp) {
Expand Down Expand Up @@ -614,9 +587,11 @@ module.exports = {
);
// let {firstname} = props
const directDestructuring =
PROPS_REGEX.test(node.init.name) &&
(utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps())
;
PROPS_REGEX.test(node.init.name) && (
utils.getParentStatelessComponent() ||
astUtil.inConstructor(context) ||
astUtil.inLifeCycleMethod(context)
);

if (thisDestructuring) {
properties = node.id.properties[i].value.properties;
Expand Down Expand Up @@ -649,7 +624,7 @@ module.exports = {
name: name,
allNames: allNames,
node: (
!isDirectProp && !inConstructor() && !inComponentWillReceiveProps() ?
!isDirectProp && !astUtil.inConstructor(context) && !astUtil.inLifeCycleMethod(context) ?
node.parent.property :
node.property
)
Expand Down Expand Up @@ -967,7 +942,7 @@ module.exports = {
const directDestructuring =
destructuring &&
PROPS_REGEX.test(node.init.name) &&
(utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps())
Copy link
Member

Choose a reason for hiding this comment

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

also, which test cases warrant this change from just checking willReceiveProps, to checking all the lifecycle methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is missing tests! Indeed we will now check more cases than just componentWillReceiveProps

(utils.getParentStatelessComponent() || astUtil.inConstructor(context) || astUtil.inLifeCycleMethod(context))
;

if (!thisDestructuring && !directDestructuring) {
Expand Down
45 changes: 44 additions & 1 deletion lib/util/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
*/
'use strict';

const LIFE_CYCLE_METHODS = ['componentWillReceiveProps', 'shouldComponentUpdate', 'componentWillUpdate', 'componentDidUpdate'];

/**
* Get properties name
* @param {Object} node - Property.
Expand Down Expand Up @@ -59,8 +61,49 @@ function findReturnStatement(node) {
return false;
}

/**
* Check if we are in a lifecycle method
* @return {boolean} true if we are in a lifecycle method, false if not
**/
function inLifeCycleMethod(context) {
let scope = context.getScope();
while (scope) {
if (
scope.block &&
scope.block.parent &&
scope.block.parent.key &&
LIFE_CYCLE_METHODS.indexOf(scope.block.parent.key.name) >= 0
) {
return true;
}
scope = scope.upper;
}
return false;
}

/**
* Check if we are in a class constructor
* @return {boolean} true if we are in a class constructor, false if not
*/
function inConstructor(context) {
let scope = context.getScope();
while (scope) {
if (
scope.block &&
scope.block.parent &&
scope.block.parent.kind === 'constructor'
) {
return true;
}
scope = scope.upper;
}
return false;
}

module.exports = {
findReturnStatement: findReturnStatement,
getPropertyName: getPropertyName,
getComponentProperties: getComponentProperties,
findReturnStatement: findReturnStatement
inConstructor: inConstructor,
inLifeCycleMethod: inLifeCycleMethod
};
17 changes: 17 additions & 0 deletions tests/lib/rules/no-unused-prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3241,6 +3241,23 @@ ruleTester.run('no-unused-prop-types', rule, {
line: 3,
column: 13
}]
}, {
code: [
'class Hello extends Component {',
' static propTypes = {',
' unused: PropTypes.bool',
' }',
' constructor(props) {',
' this.state = { something: props.something }',
' }',
'}'
].join('\n'),
parser: 'babel-eslint',
errors: [{
message: '\'unused\' PropType is defined but prop is never used',
line: 3,
column: 13
}]
}, {
code: [
'class Hello extends Component {',
Expand Down
Loading