Eslint-plugin-react: Consider deprecating `no-unused-prop-types` rule

[EvHaus]

I wrote the no-unused-prop-types rule which was merged into eslint-plugin-react in v6.2.0. Thanks for accepting it and letting me contribute to this project. I've been using this rule for many different projects with some success. However, after some more careful analysis, I am now fairly convinced that we should remove it.

Although it's very useful and can definitely help detect dead code, it's highly prone to false positives as seen via: #628, #811, #816, #819, #833, #879, #880, #884, #885, #944, #974 and many more.

I've taken a stab last week to improve the rule to fix the false positives, but it ultimately led me to the realization that in order to truly fix them, we would need to update the rule to fully track the use of props across all functions in the file. This is an incredibly difficult task given the plethora of ways that arguments can be passed into functions. Consider this worst case scenario:

class MyComponent extends Component {
    static propTypes = {
        // This is the prop that we want to make sure is tracked
        myProp: PropTypes.string
    }

    constructor (props) {
        super(props);

        // All props are passed to some class function. Technically `myProp`
        // isn't considered used yet. It's also not calling the function directly
        // but using `bind()` instead for extra complexity.
        this.doSomething.bind(this, props);
    }

    doSomething (props) {
        // This class function doesn't use `myProp` directly either, and also passes
        // `props` to some external function.
        doAnotherThing(props);
    }
}

// This function isn't even part of the component anymore
// and uses object destructuring to break up the props
const doAnotherThing = ({myProp}) => {
    // Finally the prop is used
    console.log(myProp);
};

In order to properly mark myProp as used in this case, I need to track the movement of props all the way from the constructor, into doSomething (via bind), then into doAnotherThing while being destructured (and outside the original component), and finally then I can check if a specific prop is used.

Now you have to do this for every prop in every component of your code. The ESLint rule that would support this tracking is hugely complicated and super slow. I don't think it's worth the benefits of us going down this path.

But unless we venture down this road and do it right, we will always have false positives reported.

@yannickcr @lencioni @ljharb Thoughts?

[ljharb]

It seems like a very useful rule that I'm loath to remove, but I do agree that it could have very many false positives.

[nonoroazoro]

Like @ljharb said, it's very useful and it does have probems of current implementation.
I switch the rule to "warn" for now. 😭

[zombieJ]

I face the false positives like your sample code. Currently I add comment for temporary disable the rule on the prop.
I'm not sure remove it is a good way cause it's useful when refactor the code.

[alk-jerber]

Well it would help if this rule also checks on

• nextProps (for example in componentWillReceiveProps)
• ownProps (second parameter in mapDispatchToProps)
• props that are argument of functions that have been called with this.props as parameter

I think that would already cover a big number of false positives. Overall I think it is a very useful rule which should be kept.

[tommytroylin]

@johanneserber agreed.

In my project, there are some cases that some props are used only in componentWillReceiveProps(), and now I have to set this rule to 'warn' and manually check warnings later

[yannickcr]

@EvNaverniouk like @ljharb I find this rule useful and I think it would be a shame to remove it (despite the fact that I do not use it myself, because of the false positives 😢).

Imho to make it really useful we should:

• Handle the most popular patterns used by the React community (ideally prop-types and no-unused-prop-types should handle the same patterns).
• Improve the documentation to warn about the false positives and maybe list the handled patterns and/or limitations of the rules.

[sompylasar]

The complex case of the props object being passed back and forth may be simplified by marking every defined prop as being used.

This rule would be very helpful for the top-level props tracking, especially in the stateless functions. It would better skip some "maybe used" props and reliably report "definitely unused", i.e. if there's no props object passing (except constructor super), but there's still destructuring in render.

[EvHaus]

Closing this for now. Consensus seems to be that it's useful. Hopefully the community and I can help fill in the missing gaps to reduce the false positives.

[sompylasar]

Thanks @EvNaverniouk !

[ndbroadbent]

Ha, seems like you'd have to solve the halting problem for this one.

I think this rule should be relaxed quite a bit, so that it just looks for the prop name string anywhere in the same file. I think that would solve 99% of these false positives, and it would still be quite useful.

Even if that's not the default behavior, it would be really useful as an option.

[ndbroadbent]

Actually I think johanneserber's comment suggestion would cover 99% of cases, and I would also like to add a destructuring check. e.g.:

const { foo, bar } = this.props

That would solve everything for me. Even if there are some edge-cases, that would give me enough options to rewrite the code and pass the linting.

[xareelee]

I have another case which raises an error as a false positive.

type CouponsListContainerProps = {
  errorMessage?: string,  // this props will only be used in `shouldComponentUpdate`
  ...
};

class CouponsListContainer extends React.Component {

  props: CouponsListContainerProps
  static defaultProps = {
    errorMessage: "",
  };

  shouldComponentUpdate(props: CouponsListContainerProps) {
    // we will use errorMessage without `this` keyword
    if (props.errorMessage.length > 0) {
      this.toast.show(props.errorMessage);
    }
    return true;
  }

  render = () => {...}
}

The errorMessage is an optional prop which will be used only in shouldComponentUpdate.

[miraage]

Using recompose package also triggers this as false-positive because props are used outside component (e.g. import { lifecycle } from 'recompose';)

[TrevorSayre]

export default class Users extends Component {

  static propTypes = {
    ...
    onLoad: PropTypes.func.isRequired,
  };

  async componentWillMount() {
    await Promise.all([
      this.props.onLoad(),
      this.onSearch(),
    ]);
    ...
  }

  ...

}

'onLoad' PropType is defined but prop is never used (react/no-unused-prop-types)

[TheSavior]

I know there are a lot of false positives for this rule in our codebase and unfortunately that is keeping us from turning it on. I think I'd actually prefer false negatives than false positives for this lint rule. False negatives would make me think it would be great if it was smarter and could be improved whereas false positives are annoying for teams and make it more likely it will get turned back off.

I'm not sure how feasible a shift in that direction would be, I haven't taken a close look at the code. Do others have the same perspective on false positives vs false negatives?