Adds new `no-unused-prop-types` rule #611

EvHaus · 2016-05-23T02:17:40Z

Fixes #226 by adding an new unused-prop-types rule.

The rule is based very heavily on the prop-types rule and shares a lot of the same code. For the time being, I have not made any attempts at breaking out the shared code into a reusable utility file. Please advise on the best way to do this. I was simply thinking of dropping a proptypes.js file into the utils directory with all the shared functions. If that's acceptable, let me know and I will do that.

Have tested against several React projects with great success so far. No false positive problems yet, but it needs a lot more testing against other repos. I'm hoping other users will report issues as they try it. For my needs, this rule is working great.

Can be tested by following these steps:

npm install EvNaverniouk/eslint-plugin-react#6d309e13a65a1032577ae0c98081d778f85d515a

Then enable the rule in your eslintrc:

"rules": {
    "react/unused-prop-types": 2
}

coveralls · 2016-05-23T02:19:52Z

Coverage decreased (-0.08%) to 96.686% when pulling b95b0ef on EvNaverniouk:226-unused-prop-types into 82b3aa9 on yannickcr:master.

EvHaus · 2016-05-23T02:22:51Z

I found one condition where this reports a false positive:

class Hello extends Component {
    static propTypes = {
        params: PropTypes.shape({
            id: PropTypes.string
        })
    }
    render () {
        const {params} = this.props;
        const id = (params || {}).id;
        return <span>{id}</span>;
    }
}

Not sure yet how to get (params || {}).id to get detected properly. Any advise would be welcome. I'll keep looking into it.

EDIT: I think trying to resolve it will be a losing battle of whack-a-mole. As such, I have added an ignoreShapeProp option to the rule to disable checking of shape proptypes.

coveralls · 2016-05-23T02:36:04Z

Coverage decreased (-0.08%) to 96.686% when pulling 0fce5d1 on EvNaverniouk:226-unused-prop-types into 82b3aa9 on yannickcr:master.

coveralls · 2016-05-23T02:37:59Z

Coverage decreased (-0.08%) to 96.686% when pulling 0fce5d1 on EvNaverniouk:226-unused-prop-types into 82b3aa9 on yannickcr:master.

coveralls · 2016-05-23T06:31:17Z

Coverage decreased (-0.07%) to 96.699% when pulling a2faeb6 on EvNaverniouk:226-unused-prop-types into 82b3aa9 on yannickcr:master.

mattdell · 2016-06-07T13:13:53Z

I'm going to have a look at this now but want to flag to @EvNaverniouk that the PR has conflicts. Can you fix @EvNaverniouk ?

I think the issue using the pattern const {params} = this.props is something we can live with. It would be better to find unused instances of this.props.params as a first pass and we can work on extending this for the former pattern as well.

EvHaus · 2016-06-07T15:33:51Z

Merged master into this PR to resolve conflicts. Thanks @mattdell

ljharb · 2016-06-07T16:18:33Z

@EvNaverniouk it'd be nice if you could rebase master instead, so there's no merge commits (and to squash all the "wip" commits)

EvHaus · 2016-06-07T17:58:05Z

@ljharb Rebased.

mattdell · 2016-06-10T10:20:24Z

What needs to happen to get this merged?

antialias · 2016-07-13T19:26:22Z

looks like it needs another merge / rebase

ljharb · 2016-07-13T20:39:23Z

@EvNaverniouk would you mind rebasing this?

EvHaus · 2016-07-14T22:13:55Z

@ljharb Rebase completed.

ljharb · 2016-07-14T22:52:51Z

ping @yannickcr :-)

Faradey27 · 2016-07-26T08:25:48Z

@EvNaverniouk do you plan to fix conflicts?

antialias · 2016-07-26T09:32:59Z

I resolved the conflicts between this branch and master and pushed them to my fork: antialias@dffa1f3

There were no ambiguities, i.e. I'm 100% confident that I resolved them correctly.

@yannickcr, I could submit a replacement PR or @EvNaverniouk, you could hard reset this branch to the commit from my branch and the PR would show as resolved.

EvHaus · 2016-07-26T15:22:41Z

Looks like some of my tests are failing after merging from latest master. Investigating...

EvHaus · 2016-07-26T15:25:57Z

Ok. Fixed the conflicts and rebased again. Also added a new commit to fix a bug in the Component.js utility method. I left that as a separate commit so you can clearly see it: fcd9d59

petersendidit · 2016-07-28T15:04:32Z

lib/rules/unused-prop-types.js

+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = Components.detect(function(context, components, utils) {


This should use the new eslint rule format

EvHaus · 2016-08-02T15:21:49Z

@petersendidit Updated PR to use new ESLint rule format.

dhruska · 2016-08-08T16:55:40Z

I pulled down this branch into one of my projects to give it a try, since I'm really excited about this - it worked great, except for in one component where it did not detect that I was only using a prop in componentWillReceiveProps on the nextProps object. Granted, it's probably a poor design pattern in my project that I am doing this, but the linter threw an error when I was indeed using the prop.

EvHaus · 2016-08-08T17:53:58Z

@dhruska Can you send me a copy of the code that reproduces that?

dhruska · 2016-08-08T19:01:58Z

@EvNaverniouk Let me work on getting you an example. I tried to create a simple component that would reproduce the issue and was not able to.

burabure · 2016-08-11T19:48:07Z

this will be very useful! I'll try to test this on our codebase (1M+ lines) and report back

lencioni · 2016-08-20T21:46:32Z

docs/rules/unused-prop-types.md

+  }
+});
+
+function Hello({ name }) {


I don't understand why this is included in the examples--this component has no propTypes. Did you mean to include something like this?

Hello.propTypes = { name: React.PropTypes.string.isRequired, };

EvHaus · 2016-08-21T02:41:32Z

@lencioni Updated PR to clean up the docs as per your recommendations, fixed merge conflicts and rebased.

lencioni · 2016-08-21T05:35:14Z

I think this looks good, thanks for all of your work on this rule. If this code is based off of the prop-types rule code, we currently have a bug in that rule where nested destructured props are not marked as used. (#296) It would be good to include some examples in your tests that demonstrate this.

EvHaus · 2016-08-22T02:18:18Z

@lencioni Not sure if that's what you had in mind, but I added a commented-out test at f083a13#diff-655c668b4b1964c5be4ff486d879ef45R1980 which can be enabled once #296 is fixed.

lencioni · 2016-08-23T04:01:06Z

@EvNaverniouk Thanks for doing that! That's not quite what I had in mind. My point was more that if we are introducing code that has bugs that we are aware of, it would be nice to fix those bugs before merging. And actually, I think the bug might be the inverse in this rule (e.g. nested destructuring of a prop in propTypes is incorrectly flagged).

 function Foo(props) {
   const { foo: { bar } } = props;
   return <div test={bar} />;
 }

 Foo.propTypes = {
   foo: PropTypes.shape({
     bar: PropTypes.number,
   }),
 };

Any chance you can add a test case for this, and if it fails, make it pass?

yannickcr · 2016-08-23T20:51:07Z

I think this rule already adds a lot of value even if this bug is not fixed right now. So I am not against merging this rule with the bug and fix it later on (actually I still need to figure out how to fix it in the prop-types rule).

Also I'd prefer to name this rule no-unused-prop-types to match ESLint Rule Naming Conventions.

yannickcr · 2016-08-23T20:52:17Z

Ho, and thank you for this @EvNaverniouk, this is really great work ❤️

EvHaus · 2016-08-24T03:19:21Z

@lencioni To be honest with you, fixing that bug is a bit beyond my level of understanding of how the component and proptype detection works. However, I'm happy to merge the fix in into this rule once someone solves that bug for the prop-types rule. I've subscribed to it and will keep a close eye on it once a fix is found.

@yannickcr I've renamed the rule to no-unused-prop-types and rebased again.

lencioni · 2016-08-24T03:31:02Z

@EvNaverniouk Perfect is the enemy of the good, so that sounds good to me. Thanks for your work on this!

yannickcr · 2016-08-25T19:16:36Z

Merged, thanks!

Faradey27 · 2016-09-02T20:38:54Z

thank you guys, nice rule and nice job

tristanbbq · 2016-09-15T19:14:59Z

@yannickcr: I updated to 6.2.2 and added rule as following:
"react/no-unused-prop-types": ["warn", { "customValidators": [], "skipShapeProps": true }],
But I keep getting warnings PropType is defined but props is never used for a PropType.shape.

Something I'm missing here?

EvHaus · 2016-09-15T20:07:39Z

@tristanbbq Can you please open a new issue with a code sample I can use to reproduce?

tristanbbq · 2016-09-15T20:49:07Z

@EvNaverniouk: just created #837

EvHaus mentioned this pull request May 23, 2016

Rule proposal: Check for unused propTypes #226

Closed

EvHaus force-pushed the 226-unused-prop-types branch 3 times, most recently from 46462cc to 74f0cbe Compare June 7, 2016 17:57

ljharb added enhancement new rule labels Jun 10, 2016

ljharb assigned yannickcr Jun 10, 2016

beefancohen mentioned this pull request Jun 16, 2016

Rule proposal: jsx-no-undef-props #635

Closed

aduth mentioned this pull request Jun 24, 2016

Framework: Refine ESLint configuration Automattic/wp-calypso#6294

Merged

EvHaus force-pushed the 226-unused-prop-types branch from 7b9fdb6 to 6bbb9f3 Compare July 14, 2016 22:13

EvHaus force-pushed the 226-unused-prop-types branch from 6bbb9f3 to e2dbee2 Compare July 26, 2016 15:15

petersendidit reviewed Jul 28, 2016
View reviewed changes

yannickcr force-pushed the master branch from 3f5a274 to e310ba2 Compare August 14, 2016 13:30

lencioni reviewed Aug 20, 2016
View reviewed changes

EvHaus force-pushed the 226-unused-prop-types branch from abd522f to 504bcd5 Compare August 21, 2016 02:40

EvHaus force-pushed the 226-unused-prop-types branch from 504bcd5 to f083a13 Compare August 22, 2016 02:17

Adds new no-unused-prop-types rule

8be951a

EvHaus force-pushed the 226-unused-prop-types branch from b2c8cf5 to 8be951a Compare August 24, 2016 03:16

EvHaus changed the title ~~Adds new unused-prop-types rule~~ Adds new no-unused-prop-types rule Aug 24, 2016

yannickcr merged commit ed1f168 into jsx-eslint:master Aug 25, 2016

tristanbbq mentioned this pull request Sep 15, 2016

Destructured use of property is not recognized by no-unused-prop-types #816

Closed

tristanbbq mentioned this pull request Sep 15, 2016

Rule "no-unused-prop-types" does not skip shape props properly #837

Closed

Adds new no-unused-prop-types rule #611

Adds new no-unused-prop-types rule #611

Conversation

EvHaus commented May 23, 2016 • edited

coveralls commented May 23, 2016 • edited

EvHaus commented May 23, 2016 • edited by lencioni

coveralls commented May 23, 2016 • edited

coveralls commented May 23, 2016 • edited

coveralls commented May 23, 2016 • edited

mattdell commented Jun 7, 2016

EvHaus commented Jun 7, 2016

ljharb commented Jun 7, 2016

EvHaus commented Jun 7, 2016

mattdell commented Jun 10, 2016

antialias commented Jul 13, 2016

ljharb commented Jul 13, 2016

EvHaus commented Jul 14, 2016

ljharb commented Jul 14, 2016

Faradey27 commented Jul 26, 2016

antialias commented Jul 26, 2016

EvHaus commented Jul 26, 2016

EvHaus commented Jul 26, 2016

petersendidit Jul 28, 2016

Choose a reason for hiding this comment

EvHaus commented Aug 2, 2016

dhruska commented Aug 8, 2016

EvHaus commented Aug 8, 2016

dhruska commented Aug 8, 2016

burabure commented Aug 11, 2016

lencioni Aug 20, 2016

Choose a reason for hiding this comment

EvHaus commented Aug 21, 2016

lencioni commented Aug 21, 2016

EvHaus commented Aug 22, 2016

lencioni commented Aug 23, 2016

yannickcr commented Aug 23, 2016

yannickcr commented Aug 23, 2016 • edited

EvHaus commented Aug 24, 2016

lencioni commented Aug 24, 2016

yannickcr commented Aug 25, 2016

Faradey27 commented Sep 2, 2016

tristanbbq commented Sep 15, 2016 • edited

EvHaus commented Sep 15, 2016

tristanbbq commented Sep 15, 2016 • edited

Adds new `no-unused-prop-types` rule #611

Adds new `no-unused-prop-types` rule #611

EvHaus commented May 23, 2016 •

edited

coveralls commented May 23, 2016 •

edited

EvHaus commented May 23, 2016 •

edited by lencioni

coveralls commented May 23, 2016 •

edited

coveralls commented May 23, 2016 •

edited

coveralls commented May 23, 2016 •

edited

yannickcr commented Aug 23, 2016 •

edited

tristanbbq commented Sep 15, 2016 •

edited

tristanbbq commented Sep 15, 2016 •

edited