no-unused-props: not ignoring components with no used props #1303

DianaSuvorova · 2017-07-14T20:20:15Z

This is tackling one of the many issues with the rule (issue #1162).

The problem was that the rule simply ignored the components that had no used properties. Once this condition was removed I had to fix some tests and remove some other (that were never actually worked correctly but were passing because of the component had no used props and thus was excluded from validation).

This change might fix few more false positives that users reported. I am planning to go through the list of open issues for the rule and close/fix some more after I get some feedback on my change.

DianaSuvorova · 2017-07-14T20:24:52Z

tests/lib/rules/no-unused-prop-types.js

@@ -784,54 +764,6 @@ ruleTester.run('no-unused-prop-types', rule, {
      ].join('\n')
    }, {
      code: [
-        'const statelessComponent = (props) => {',


First of all for some reason the same test were copy-pasted 4 times. I had to remove this test all together because it was never validated correctly. Current implementation is not able to detect correctly nested definition of components (subRender technically is a react component). I don't think this is a common/idiomatic react style so I assume we shouldn't support it.

It's not the same test; I see 4 different variations: arrow, non-arrow, and destructured, and non-destructured.

Got it. Makes sense. Is ti ok if I delete these tests though. Do you agree that this rule technically doesn't support this use case.

I think that it ideally should support this use case, but our mechanism of detecting components may make it difficult.

If you ask me, the fact that certain cases are not supported could also be tested. Of course, with a test name which clearly states "testing unsupported case" and assertions that clearly check for false positives or other signs of "not supported"-ness.

sure, that seems reasonable too.

DianaSuvorova · 2017-07-14T20:25:41Z

tests/lib/rules/no-unused-prop-types.js

@@ -3215,6 +3148,23 @@ ruleTester.run('no-unused-prop-types', rule, {
        line: 11,
        column: 8
      }]
+    }, { // None of the props are used
+      code: [


this is the test reported for the issue

ljharb

Thanks for working on this!

ljharb · 2017-07-14T22:46:09Z

lib/util/Components.js

    if (usedPropTypes[id]) {
-      list[j].usedPropTypes = (list[j].usedPropTypes || []).concat(usedPropTypes[id]);


this change doesn't seem like an improvement; it increases the number of mutations.

Yeah, I agree it is not exactly an improvement.
Reason I added it - so usedPropTypes it is an empty array when there is no usedPropTypes instead of null. Otherwise anywhere we loop through it we would need to check if it is an array or not.
I can remove it and add few checks in the rule file - I don't mind either way. Let me know what is preferable.

I see what you mean. Could you point to the other lines you'd need an || [] on, if you reverted these lines?

Just one place actually:
lib/rules/no-unused-prop-types.js:236

function isPropUsed(node, prop) { for (let i = 0, l = node.usedPropTypes.length; i < l; i++) { const usedProp = node.usedPropTypes[i]; if ( ...

I'm ambivalent; my general preference is to avoid mutations, but I don't feel super strongly about it. I'll leave it up to you :-)

ljharb · 2017-07-14T22:48:41Z

tests/lib/rules/no-unused-prop-types.js

+      code: [
+        'var Hello = createReactClass({',
+        '  propTypes: {',
+        '    unused: PropTypes.string',


The test case specifically had React.PropTypes, and React.createClass - are these not necessary to trigger the bug?

This didn't matter. But I updated the test so it is identical to the one reported.

ljharb · 2017-07-15T04:18:18Z

lib/util/Components.js

    if (usedPropTypes[id]) {
-      list[j].usedPropTypes = (list[j].usedPropTypes || []).concat(usedPropTypes[id]);


I see what you mean. Could you point to the other lines you'd need an || [] on, if you reverted these lines?

ljharb · 2017-07-15T04:18:28Z

tests/lib/rules/no-unused-prop-types.js

@@ -1387,15 +1319,16 @@ ruleTester.run('no-unused-prop-types', rule, {
      code: [
        'const PagingBlock = function(props) {',
        '  return (',
-        '    <SomeChild {...props} />',
+        '    <span>{...props}</span>',


Why this change?

to detect usage of a spread operator (that is handled in JSXSpreadChild I added to the rule) SomeChild has to be a valid react component (this is a babel-eslint thing). Originally when component was invalid, babel didn't detect any props passed to it.

There's no need to use babel-eslint for spread props; so let's revert this change and keep it on the default parser.

as far as I can tell spread works with es6 but jsx spread inside a component does not. Simply removing babel and adding parserOptions fails with : message: 'Parsing error: Unexpected token ...',

I am happy to try any suggestions you might have.

{ code: [ 'const PagingBlock = function(props) {', ' return (', ' {...props}', ' );', '};', 'PagingBlock.propTypes = {', ' nextPage: PropTypes.func.isRequired,', ' previousPage: PropTypes.func.isRequired,', '};' ].join('\n'), parserOptions: Object.assign({}, parserOptions, {ecmaVersion: 2017}) }

error:

AssertionError: Should have no errors but had 1: [ { ruleId: null, fatal: true, severity: 2, source: ' {...props}', message: 'Parsing error: Unexpected token ...', line: 3, column: 12 } ] + expected - actual -1 +0 at testValidTemplate (node_modules/eslint/lib/testers/rule-tester.js:415:20) at Context.RuleTester.it (node_modules/eslint/lib/testers/rule-tester.js:545:25)

I'm a bit confused… {...props} in a child context isn't anything; that shouldn't work. Spreading is a JSX attribute thing, and a JS iterable thing, and a JS object thing - but not a child thing.

I guess I have to add that the reason it worked before is that the component was invalid by itself so the parser wouldn't try to parse the props of SomeChild. And that way the linter didn't know that props are actually used. That was ok before this change (where components with 0 used props were simply not validated). If we want to validate components with no props used, we need to make sure that where they are actually used, the code is syntactically valid.

In other words if I keep this test as it is it would fail with nextPage and PreviousPage are not used error.

I'm a bit confused… {...props} in a child context isn't anything; that shouldn't work. Spreading is a JSX attribute thing, and a JS iterable thing, and a JS object thing - but not a child thing.

I guess it is getting late... I am not sure I understand what you mean .. But I agree regrdless😃

{...props} is invalid jsx - if babel-eslint parses it, it's a bug in babel-eslint.

Oh I see, good point. Let me update the test so SomeChild is a thing then. I will look into creating the issue with babel-eslint.

The fix was easy. Changing type from JSXChildSpread to JSXAttribute spread...Thanks for catching this. I will go read about JSXChildSpread though... maybe it is some beta feature...

ljharb · 2017-07-15T04:18:41Z

tests/lib/rules/no-unused-prop-types.js

        ' );',
        '};',

        'PagingBlock.propTypes = {',
        '  nextPage: PropTypes.func.isRequired,',
        '  previousPage: PropTypes.func.isRequired,',
        '};'
-      ].join('\n')
+      ].join('\n'),
+      parser: 'babel-eslint'


what in this test needs babel-eslint?

this component uses JSX spread few lines above.So it needs babel.

jsx spread works in ES6 or above in the default parser with parserOptions: { jsx: true } - there's definitely no need for babel-eslint for it.

responded above

ljharb · 2017-07-15T04:18:59Z

tests/lib/rules/no-unused-prop-types.js

+        '  }',
+        '});'
+      ].join('\n'),
+      parser: 'babel-eslint',


similarly here, what in this test needs babel-eslint? this test seems like it'd work with ES6.

yes, here es6 would do.

DianaSuvorova · 2017-07-15T08:01:09Z

@ljharb, looks like this will take some more time. Neither babel nor es6 parser gives any indication that {...props} is being used. And it has nothing to do with component being a thing (thanks for catching that issue with invalid spread for babel ).

So this

 return <SomeComponent {...props} />;

is parsed into:

note 0 children in arguments. If I use props without spread, I see children there.

What dependency do you think this problem starts from? Would it be an eslint/espree issue?

Update: Nevermind, it shouldn't have children it should have props. I will look into that and update PR. Thanks for your help!!

ljharb · 2017-07-15T16:00:02Z

tests/lib/rules/no-unused-prop-types.js

@@ -1317,9 +1318,16 @@ ruleTester.run('no-unused-prop-types', rule, {
    }, {
      // `no-unused-prop-types` rest param props in jsx expressions - [Issue #885]
      code: [
+        'const SomeComponent = funciton(props) {',


will be gone. Thanks.

ljharb · 2017-07-15T16:12:10Z

lib/util/Components.js

    if (usedPropTypes[id]) {
-      list[j].usedPropTypes = (list[j].usedPropTypes || []).concat(usedPropTypes[id]);


I'm ambivalent; my general preference is to avoid mutations, but I don't feel super strongly about it. I'll leave it up to you :-)

ljharb · 2017-07-15T16:16:19Z

tests/lib/rules/no-unused-prop-types.js

@@ -3148,24 +3148,7 @@ ruleTester.run('no-unused-prop-types', rule, {
        line: 11,
        column: 8
      }]
-    }, { // None of the props are used
-      code: [
-        'var Hello = createReactClass({',


In this case, the duplicated test isn't a duplicate; one imports React, and the other doesn't, and it's intentional to check both.

Ok I will bring this back.

ljharb · 2017-07-15T16:17:02Z

tests/lib/rules/no-unused-prop-types.js

@@ -784,54 +764,6 @@ ruleTester.run('no-unused-prop-types', rule, {
      ].join('\n')
    }, {
      code: [
-        'const statelessComponent = (props) => {',


It's not the same test; I see 4 different variations: arrow, non-arrow, and destructured, and non-destructured.

DianaSuvorova · 2017-07-15T19:20:09Z

lib/rules/no-unused-prop-types.js

@@ -234,8 +233,9 @@ module.exports = {
     * @returns {Boolean} True if the prop is used, false if not.
     */
    function isPropUsed(node, prop) {
-      for (let i = 0, l = node.usedPropTypes.length; i < l; i++) {
-        const usedProp = node.usedPropTypes[i];
+      const usedPropTypes = node.usedPropTypes || [];


I reverted that change from Components file.

DianaSuvorova · 2017-07-15T19:28:30Z

@ljharb sorry for so many updates. I squashed my change into 1 commit and it is ready for re-review. Thanks!

ljharb · 2017-07-16T15:40:41Z

lib/rules/no-unused-prop-types.js

@@ -897,6 +897,13 @@ module.exports = {
        }
      },

+      JSXSpreadAttribute: function(node) {
+        const component = components.get(utils.getParentComponent());


It seems super weird to me that getParentComponent infers the node rather than taking it as an argument (not your code ofc, just thought I'd comment)

ljharb · 2017-07-16T15:41:41Z

tests/lib/rules/no-unused-prop-types.js

-        '  }',
-        '}',
-        'Comp2.propTypes = {',
-        '  prop2: PropTypes.arrayOf(Comp1.propTypes.prop1)',


This test implies that Comp1's prop1 prop should be marked as used, because Comp2 references its propType; but in fact I think it shouldn't - could you move this test to be invalid instead of just deleting it?

ljharb · 2017-07-16T15:42:20Z

tests/lib/rules/no-unused-prop-types.js

@@ -784,54 +764,6 @@ ruleTester.run('no-unused-prop-types', rule, {
      ].join('\n')
    }, {
      code: [
-        'const statelessComponent = (props) => {',


I think that it ideally should support this use case, but our mechanism of detecting components may make it difficult.

ljharb

LGTM, thanks for the iteration!

DianaSuvorova · 2017-07-19T16:03:08Z

@lencioni, when would you be able to take a look at this PR? This is my first PR for the repo so I am not sure what is the normal turn around here.

Thank you,
Diana

ljharb · 2017-07-20T01:44:25Z

I'll give it another day, then I'll merge it if there's no objections by then.

ljharb · 2017-07-20T23:56:43Z

Sorry for the inconvenience; I've just merged a refactor to start using arrow functions. Would you mind rebasing latest master? I'll merge ASAP after that's done.

DianaSuvorova · 2017-07-21T00:30:52Z

@ljharb, no problem. Should be all set.

DianaSuvorova commented Jul 14, 2017

View reviewed changes

ljharb reviewed Jul 14, 2017

View reviewed changes

ljharb reviewed Jul 15, 2017

View reviewed changes

DianaSuvorova commented Jul 15, 2017

View reviewed changes

DianaSuvorova force-pushed the noUsedProps branch from 8cde4a1 to b7befbc Compare July 15, 2017 19:25

no ignoring components with no used props

9d0707e

DianaSuvorova force-pushed the noUsedProps branch from b7befbc to 9d0707e Compare July 15, 2017 19:27

ljharb reviewed Jul 16, 2017

View reviewed changes

moving test into invalid

430a89f

ljharb approved these changes Jul 16, 2017

View reviewed changes

ljharb requested review from yannickcr and lencioni July 16, 2017 16:46

Merge branch 'master' into noUsedProps

64e837b

ljharb merged commit ef2054b into jsx-eslint:master Jul 21, 2017

ljharb mentioned this pull request Jul 21, 2017

no-unused-prop-types does not trigger if class does not use any properties #1162

Closed

DianaSuvorova mentioned this pull request Jul 21, 2017

no-unused-prop-types - count all properties as used when met {...this.props} #879

Closed

dependencies bot mentioned this pull request Aug 25, 2017

Update eslint-plugin-react from 7.1.0 to 7.3.0 chauncey-garrett/dotfiles#13

Open

renovate bot mentioned this pull request Feb 2, 2018

Update dependency eslint-plugin-react to v7 JimmyLv/Haiku#34

Open

1 task

This was referenced Oct 25, 2018

Update dependency eslint-plugin-react to v7 efcsydney/efcsydney-roster#502

Merged

Update dependency eslint-plugin-react to v7 CaptainGlac1er/StudentHomeworkProject#38

Open

renovate bot mentioned this pull request Nov 7, 2018

chore(deps): update dependency eslint-plugin-react to v7 vgmtv/rsvg#12

Merged

renovate bot mentioned this pull request Nov 23, 2018

chore(deps): update dependency eslint-plugin-react to v7 JLHwung/tianzige#31

Open

1 task

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 mesosphere/reactjs-components#475

Closed

1 task

This was referenced Jan 8, 2019

Update dependency eslint-plugin-react to v7 SonyaMoisset/Team-Treehouse_Training#8

Closed

Update dependency eslint-plugin-react to v7 - autoclosed amytych/react-escape-outside#10

Merged

		if (usedPropTypes[id]) {
		list[j].usedPropTypes = (list[j].usedPropTypes \|\| []).concat(usedPropTypes[id]);

no-unused-props: not ignoring components with no used props #1303

no-unused-props: not ignoring components with no used props #1303

Conversation

DianaSuvorova commented Jul 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova Jul 14, 2017 • edited

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova commented Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova commented Jul 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

DianaSuvorova commented Jul 19, 2017

ljharb commented Jul 20, 2017

ljharb commented Jul 20, 2017

DianaSuvorova commented Jul 21, 2017

DianaSuvorova commented Jul 14, 2017 •

edited

DianaSuvorova Jul 14, 2017 •

edited

DianaSuvorova Jul 15, 2017 •

edited

DianaSuvorova Jul 15, 2017 •

edited

DianaSuvorova commented Jul 15, 2017 •

edited