eslint-plugin-react: no-unused-prop-types false positives

Counting used nextProps in componentWillReceiveProps is a clear win for this rule, certainly.

One more example of false positive:

const Button = React.createClass({
    displayName: 'Button',

    propTypes: {
        name: React.PropTypes.string.isRequired,
        isEnabled: React.PropTypes.bool.isRequired
    },

    render() {
        const item = this.props;
        const disabled = !this.props.isEnabled;
        return (
            <div>
                <button type="button" disabled={disabled}>{item.name}</button>
            </div>
        );
    }
});

Here name is marked as unused. In fact, all properties accessed via temporary local variable will be flagged as unused.

Its a grand ambition to have a rule like this. JavaScript is a highly dynamic language and there is no way a static checker can detect unused props reliably. Turning it off may be the only sane and time-practical recourse. My 2 cents of course.

Similar to the componentWillReceiveProps example, ownProps used in mapStateToProps(while connecting with the redux store) are also not counted as used. E.g.

function mapStateToProps(store, ownProps) {
  return {
    items: getItems(store, ownProps.visible),
  };
}

@ljharb - do you think counting this can be considered?

  "rules": {
    "react/no-unused-prop-types": 0
  },

It is a pity that is the unique working way to fix that 😦 .

mapStateToProps is a lot harder because it’s redux-specific, and not tied to the component.

However, wouldn’t a prop only used in mapStateToProps be a prop you want to not pass to the component?

It wasn’t meant to be a spam

Didn’t mean you specifically. We’re all replying to a closed thread (including myself).

as I typed it up here

Yes and as @ljharb pointed it out, its not a legit case. What I am saying is the maintainers will be more inclined to take up something that is not being debated in a closed thread with a use case that can be argued as not the most compelling example of a valid use case. Didn’t mean anything personally.

a linter can’t really determine everything

That sounds familiar. Did someone say so earlier? 😛

As much as I hate getting slapped around by a linter, I agree with @ljharb here. getLocations is poorly designed in this case. You can easily change the call in constructor to this.getLocations(props) and avoid this whole issue in the first place.

I like linting as much as the next person but in this case, the use case itself is not compelling. The authors do listen if there is a good use case. And linting is purely an opt-in thingy. You lint because you want your code to really look good, not because you want some figure head to give a seal of OK.

That said, there is no point in spamming a closed thread. If there are valid concerns, then a new issue should be opened.

I’m telling you that as a maintainer of this plugin, it’s not possible to do it reliably, because it’s not always pulling off of this.props. Meaning, if there’s any chance whatsoever that the destructuring is not off of a props object, then we can’t pay attention to it. I’m sure we could cover your specific case, but doing so would risk doing the wrong thing for other cases, so we will not be covering your particular case. The fact that it’s an antipattern means that you have a better way to write your code anyways that remove the need for extra changes in this plugin.

@ljharb, @ivarni - I agree with your point. What I was trying to achieve by keeping the “only being used in mapStateToProps” prop in propTypes was to ensure that I can use React PropTypes validations to enforce the requirement and the data type of that prop along with a default value if needed. Since this prop is not a part of the props finally being used, one can better remove it from the propTypes definition, at the cost of missing the validation and default value.

Having said that, the comments above are missing a fine detail here, the object returned by mapStateToProps is not really a “replacement” props object, since it is “merged” to the original props object. While I don’t agree that a prop being used only in mapStateToProps need not be passed to the component(given the example above), it’s perfectly valid to say that mapStateToProps is not a part of the actual React component class and thus trying to validate inputs to mapStateToProps using the React component PropType validations is not usual.

Thank you for the fruitful discussion. I’ll just disable the rule with inline comments.

The propTypes describe what props the component needs in order to function and mapStateToProps is not really part of the component’s class so you’d need some different mechanism for ensuring that the redux store actually contains the properties you need in order to transform them to component props.

React will also only check props that are actually passed on to the component itself and not those passed to mapStateToProps.