-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Description
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?