From e672e0fc3d7e349c1d968ca251cbe421a5947be2 Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Tue, 30 May 2017 15:29:53 +0300 Subject: [PATCH 1/6] Add a bunch of failing tests. The valid tests have two props, one is used in the render() function and the other in nextProps using the dot operator in the folllowing lifecycle methods: * shouldComponentUpdate * componentWillUpdate * componentDidUpdate (uses prevProps instead of nextProps). The invalid tests are the same as the valid tests but without the render() method. In here there should be an error that one of the props is unused, but there currently is no error. --- tests/lib/rules/no-unused-prop-types.js | 187 +++++++++++++++++++++++- 1 file changed, 186 insertions(+), 1 deletion(-) diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js index 783ff42e1e..c8b50a5598 100644 --- a/tests/lib/rules/no-unused-prop-types.js +++ b/tests/lib/rules/no-unused-prop-types.js @@ -1484,6 +1484,127 @@ ruleTester.run('no-unused-prop-types', rule, { ' c: React.PropTypes.string.isRequired,', '};' ].join('\n') + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' shouldComponentUpdate (props) {', + ' if (props.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentWillUpdate (props) {', + ' if (props.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentWillReceiveProps (nextProps) {', + ' const {foo} = nextProps;', + ' if (foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' shouldComponentUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentWillUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentDidUpdate (prevProps) {', + ' if (prevProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint' } ], @@ -2430,7 +2551,71 @@ ruleTester.run('no-unused-prop-types', rule, { line: 3, column: 16 }] - }/* , { + }, { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentWillUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint', + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 4, + column: 10 + }] + }, , { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' shouldComponentUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint', + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 4, + column: 10 + }] + }, , { + code: [ + 'class Hello extends Component {', + ' static propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + ' };', + '', + ' componentDidUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint', + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 4, + column: 10 + }] + } + /* , { // Enable this when the following issue is fixed // https://github.com/yannickcr/eslint-plugin-react/issues/296 code: [ From 6f2dbb787dab19ffa4e12e674a86e7452fb1ff7d Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Tue, 30 May 2017 15:30:18 +0300 Subject: [PATCH 2/6] Fix the invalid test cases added in the previous commit by: * Extending the list of lifecycle methods * Checking also for the `prevProps` parameter name Currently for this rule to work, the user has to name the parameter either: * nextProps * prevProps * props --- lib/rules/no-unused-prop-types.js | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/rules/no-unused-prop-types.js b/lib/rules/no-unused-prop-types.js index 6eb5776d9f..72ff24f7c1 100644 --- a/lib/rules/no-unused-prop-types.js +++ b/lib/rules/no-unused-prop-types.js @@ -16,8 +16,10 @@ var annotations = require('../util/annotations'); // Constants // ------------------------------------------------------------------------------ -var DIRECT_PROPS_REGEX = /^props\s*(\.|\[)/; -var DIRECT_NEXT_PROPS_REGEX = /^nextProps\s*(\.|\[)/; +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 @@ -78,15 +80,16 @@ module.exports = { } /** - * Check if we are in a class constructor + * Check if we are in a lifecycle method * @return {boolean} true if we are in a class constructor, false if not **/ - function inComponentWillReceiveProps() { + function inLifeCycleMethod() { var scope = context.getScope(); while (scope) { if ( scope.block && scope.block.parent && - scope.block.parent.key && scope.block.parent.key.name === 'componentWillReceiveProps' + scope.block.parent.key && + LIFE_CYCLE_METHODS.includes(scope.block.parent.key.name) ) { return true; } @@ -106,8 +109,7 @@ module.exports = { node.object.type === 'ThisExpression' && node.property.name === 'props' ); var isStatelessFunctionUsage = node.object.name === 'props'; - var isNextPropsUsage = node.object.name === 'nextProps' && inComponentWillReceiveProps(); - return isClassUsage || isStatelessFunctionUsage || isNextPropsUsage; + return isClassUsage || isStatelessFunctionUsage || inLifeCycleMethod(); } /** @@ -511,16 +513,17 @@ module.exports = { function getPropertyName(node) { var isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node)); var isDirectNextProp = DIRECT_NEXT_PROPS_REGEX.test(sourceCode.getText(node)); + var isDirectPrevProp = DIRECT_PREV_PROPS_REGEX.test(sourceCode.getText(node)); var isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component(); var isNotInConstructor = !inConstructor(node); - var isNotInComponentWillReceiveProps = !inComponentWillReceiveProps(); - if ((isDirectProp || isDirectNextProp) + var isNotInLifeCycleMethod = !inLifeCycleMethod(); + if ((isDirectProp || isDirectNextProp || isDirectPrevProp) && isInClassComponent && isNotInConstructor - && isNotInComponentWillReceiveProps) { + && isNotInLifeCycleMethod) { return void 0; } - if (!isDirectProp && !isDirectNextProp) { + if (!isDirectProp && !isDirectNextProp && !isDirectPrevProp) { node = node.parent; } var property = node.property; From ebc6778a622b1d65dd95c2cceb060724df595744 Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Wed, 31 May 2017 08:06:37 +0300 Subject: [PATCH 3/6] Fix typos (linting error) --- tests/lib/rules/no-unused-prop-types.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js index c8b50a5598..dd406baedc 100644 --- a/tests/lib/rules/no-unused-prop-types.js +++ b/tests/lib/rules/no-unused-prop-types.js @@ -2572,7 +2572,7 @@ ruleTester.run('no-unused-prop-types', rule, { line: 4, column: 10 }] - }, , { + }, { code: [ 'class Hello extends Component {', ' static propTypes = {', @@ -2593,7 +2593,7 @@ ruleTester.run('no-unused-prop-types', rule, { line: 4, column: 10 }] - }, , { + }, { code: [ 'class Hello extends Component {', ' static propTypes = {', From 8983d64d4a1bc487b532b640ce20ce8c2220b48d Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Wed, 31 May 2017 08:14:31 +0300 Subject: [PATCH 4/6] Duplicate the test cases with the default parser. --- tests/lib/rules/no-unused-prop-types.js | 165 ++++++++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js index dd406baedc..a31330303e 100644 --- a/tests/lib/rules/no-unused-prop-types.js +++ b/tests/lib/rules/no-unused-prop-types.js @@ -1504,6 +1504,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' shouldComponentUpdate (props) {', + ' if (props.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') }, { code: [ 'class Hello extends Component {', @@ -1524,6 +1542,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' componentWillUpdate (props) {', + ' if (props.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') }, { code: [ 'class Hello extends Component {', @@ -1545,6 +1581,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' componentWillReceiveProps (props) {', + ' if (props.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') }, { code: [ 'class Hello extends Component {', @@ -1565,6 +1619,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' shouldComponentUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') }, { code: [ 'class Hello extends Component {', @@ -1585,6 +1657,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' componentWillUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') }, { code: [ 'class Hello extends Component {', @@ -1605,6 +1695,24 @@ ruleTester.run('no-unused-prop-types', rule, { '}' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'class Hello extends Component {', + ' componentDidUpdate (prevProps) {', + ' if (prevProps.foo) {', + ' return true;', + ' }', + ' }', + '', + ' render() {', + ' return (
{this.props.bar}
);', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n') } ], @@ -2572,6 +2680,25 @@ ruleTester.run('no-unused-prop-types', rule, { line: 4, column: 10 }] + }, { + code: [ + 'class Hello extends Component {', + ' componentWillUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n'), + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 19, + column: 10 + }] }, { code: [ 'class Hello extends Component {', @@ -2593,6 +2720,25 @@ ruleTester.run('no-unused-prop-types', rule, { line: 4, column: 10 }] + }, { + code: [ + 'class Hello extends Component {', + ' shouldComponentUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n'), + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 19, + column: 10 + }] }, { code: [ 'class Hello extends Component {', @@ -2614,6 +2760,25 @@ ruleTester.run('no-unused-prop-types', rule, { line: 4, column: 10 }] + }, { + code: [ + 'class Hello extends Component {', + ' componentDidUpdate (nextProps) {', + ' if (nextProps.foo) {', + ' return true;', + ' }', + ' }', + '}', + 'Hello.propTypes = {', + ' foo: PropTypes.string,', + ' bar: PropTypes.string,', + '};' + ].join('\n'), + errors: [{ + message: '\'bar\' PropType is defined but prop is never used', + line: 19, + column: 10 + }] } /* , { // Enable this when the following issue is fixed From dda84f406e3942ebbeef28fae1f321323fd519ea Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Fri, 2 Jun 2017 08:30:07 +0300 Subject: [PATCH 5/6] Fix the line number and column on which the error appears in the invalid test cases --- tests/lib/rules/no-unused-prop-types.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js index a31330303e..fbad06f427 100644 --- a/tests/lib/rules/no-unused-prop-types.js +++ b/tests/lib/rules/no-unused-prop-types.js @@ -2696,8 +2696,8 @@ ruleTester.run('no-unused-prop-types', rule, { ].join('\n'), errors: [{ message: '\'bar\' PropType is defined but prop is never used', - line: 19, - column: 10 + line: 10, + column: 8 }] }, { code: [ @@ -2736,8 +2736,8 @@ ruleTester.run('no-unused-prop-types', rule, { ].join('\n'), errors: [{ message: '\'bar\' PropType is defined but prop is never used', - line: 19, - column: 10 + line: 10, + column: 8 }] }, { code: [ @@ -2776,8 +2776,8 @@ ruleTester.run('no-unused-prop-types', rule, { ].join('\n'), errors: [{ message: '\'bar\' PropType is defined but prop is never used', - line: 19, - column: 10 + line: 10, + column: 8 }] } /* , { From 309d37cac5d288f10193f72e8915fba7770c0b4f Mon Sep 17 00:00:00 2001 From: Joachim Seminck Date: Fri, 2 Jun 2017 08:32:47 +0300 Subject: [PATCH 6/6] Replace .includes with .indexOf --- lib/rules/no-unused-prop-types.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-unused-prop-types.js b/lib/rules/no-unused-prop-types.js index 72ff24f7c1..08b3a06027 100644 --- a/lib/rules/no-unused-prop-types.js +++ b/lib/rules/no-unused-prop-types.js @@ -89,7 +89,7 @@ module.exports = { if ( scope.block && scope.block.parent && scope.block.parent.key && - LIFE_CYCLE_METHODS.includes(scope.block.parent.key.name) + LIFE_CYCLE_METHODS.indexOf(scope.block.parent.key.name) >= 0 ) { return true; }