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

[Eschon]

The no-unused-prop-types rule reports false positives for me.
Here's my code:

import React from 'react';
import Buddy from './Buddy';
import Spinner from './Spinner';
import BuddyBox from './BuddyBox';
import '../css/buddies.css';

export default function Buddies(props) {
  let buddies = props.activityBuddies.map(item => (
    <BuddyBox key={item.id} {...item} endFriendship={props.endFriendship} />
  ));

  if (buddies.length === 0) {
    buddies = <Spinner />;
  }

  return (
    <div className="buddies">
      <div className="buddies--content">
        {buddies}
      </div>
      <div className="buddies--sidebar">
        <div className="buddies--add">
          {props.friendrequests && props.friendrequests.map(item => (
            <div key={item.id}>
              <Buddy
                name={item.username}
                profilePicture={item._links.image.href}
                timestamp={item.last_sign_in_at}
              />
              <button
                className="buddies--confirm-button"
                onClick={() => {
                  props.handleFriendrequest(item._links.confirm.href);
                }}
              >
                Confirm
              </button>
              <button
                className="buddies--decline-button"
                onClick={() => {
                  props.handleFriendrequest(item._links.decline.href);
                }}
              >
                Decline
              </button>
            </div>
          ))}
        </div>
      </div>
    </div>
  );
}

Buddies.propTypes = {
  activityBuddies: React.PropTypes.array,
  endFriendship: React.PropTypes.func,
  friendrequests: React.PropTypes.array,
  handleFriendrequest: React.PropTypes.func,
};

The no-unused-prop-types-Rule shows an error for both the endFriendship and the handleFriendrequest props but both are used in my component.

I use eslint 3.4.0 and eslint-plugin-react 6.2.0.
I use the no-unused-prop-types-Rule as part of eslint-config-airbnb 11.0.0.
In the config it is set like this:

'react/no-unused-prop-types': ['error', {
  customValidators: [
  ],
  skipShapeProps: false,
}],

[feimosi]

I'm also facing a false positive error, although in a bit different case, when using destructuring:
function ListItem({ item }) {}
It highlights every property of the shape of item.

EDIT: Actually, there's already an issue for that: #816

[victor-homyakov]

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.

[perrin4869]

I'm using props in the constructor and getting a false positive too:

class Foo extends React.Component {
  static propTypes = {
    foo: PropTypes.func.isRequired,
  }

  constructor(props) {
    super(props);

    const { foo } = props;
    this.message = foo('blablabla');
  }

  render() {
    return <div>{this.message}</div>;
  }
}

This avoids multiple unnecessary calls of foo on every render... But it foo gets marked as unused

[ljharb]

@perrin4869 fwiw, your example will break if new props are passed; you'd need to put the same constructor code in componentWillReceiveProps, which is generally why instance vars are a bad practice in React. foo should be called on every render because it might be different, and if it's expensive, the caller of <Foo> should handle caching, not Foo itself.

[perrin4869]

@ljharb I should have mentioned I guess, that in my specific case, foo is always the same...

[ljharb]

Just because you currently always pass the same one doesn't mean that's a good assumption to depend on.

[stevenla]

I get false positives in this scenario:

class MyComponent extends Component {
  static propTypes = {
    error: PropTypes.string,
  };

  componentWillReceiveProps(nextProps) {
    if (nextProps.error) {
      alert(nextProps.error);
    }
  }
}

error is flagged as a prop that is defined but never used. This example is a bit contrived but it can still happen in practice.

[ivarni]

I've a legit usecase for that example. We've a HOC that handles validation and gives a submitted prop to its wrapped component once there has been an attempt at submitting the form. We're doing stuff in componentWillReceiveProps based on wether we're going to receive a true value in nextProps.submitted and had that exact false positive.

[ljharb]

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

[prithsharma]

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?

[ljharb]

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?

[klaygomes]

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

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

[prithsharma]

@ljharb - ^^ why do you say that?
May be I am missing something - if there is a prop that I need to use while mapping state to props(e.g. passing to a selector), is there a better way to do it?
FYI - the prop in question isn't in the store, if that's how the selector could have got hold of it.

[mrchief]

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.

[ljharb]

@prithsharma map state to props is meant to take wrapper props and state, and use them to construct a replacement props object. If the wrapped component does not need a prop, that prop should not remain in the replacement props - no other words, if the only use is in mapStateToProps, why would the wrapped component need it at all?

[ivarni]

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.

[prithsharma]

@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.

[DianaSuvorova]

By now all of the issues reported here have been fixed.
Except for the first one (reported by @Eschon) where only one false positive remains handleFriendrequest.This prop is only used inside a list of components that is being generated inside a map function.
I think generally suggested react style for this is to have a separate listItem component. So i don't think we are going to fix this use case.

[niksajanjic]

I get false positive in this case:

class Locations extends Component {
  constructor (props) {
    super(props)

    this.getLocations()
  }

  componentWillReceiveProps (nextProps) {
    if (nextProps.selectedZone !== this.props.selectedZone) this.getLocations(nextProps)
  }

  getLocations (nextProps) {
    const { dispatch, selectedZone } = nextProps || this.props

    dispatch(getLocations(selectedZone))
  }

  render(){ ... }
}

I get false positive for dispatch. Basically, because I'm destructuring dispatch from nextProps || this.props I get this false positive. It's easy to change this code to remove error, but I'm putting this example here as it might be a legit false positive error that needs fixing. It is obvious that dispatch is still used from nextProps being passed from componentWillReceiveProps, but if the function gets called with nextProps being anything other than undefined, null or nextProps then the error would be correct. In this case that scenario will not happen.

[ljharb]

tbh that seems like just a poor design for getLocations; you should call it with this.props instead of including the fallback.

[dzek69]

@ljharb it may be a weird code (I think dispatch won't ever change anyway if that's redux), but the discussion is about detecting the prop usage, which should be possible here, not about the quality of code.

[ljharb]

I think it's a bit of both; a linter can't really determine everything - in this case, your code is branching between "sometimes props, and sometimes a random object" - this plugin shouldn't be paying attention to properties of a random object.

[dzek69]

IMO it should be still possible to do via static analysis.

[ljharb]

I don't think it is; nextProps must be ignored, and we can't know when getLocation will be called with one or the other.

In general, passing the props object around itself is an antipattern; partly because it inhibits linting, but partly because passing around bags of things is less clear than passing around specific things.

[dzek69]

it doesn't matter when as long getLocation is a method of Locations class and "this.props" is used inside.

and again - it's not about something being an antipattern, but possibility of doing static analysis in this particular case

[ljharb]

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.

[dzek69]

It's not my case :) I just joined the conversation.

[mrchief]

a linter can't really determine everything

That sounds familiar. Did someone say so earlier? :stuck_out_tongue:

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.

[niksajanjic]

It wasn't meant to be a spam, as I typed it up here:

It's easy to change this code to remove error, but I'm putting this example here as it might be a legit false positive error that needs fixing.

[mrchief]

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.

[misterbridge]

I still have that kind of issue with the following code, that might be poor design, but I don't think so, feel free to tell me if it is just that:

static propTypes = {
  // That next line needs eslint-disable-line react/no-unused-prop-types
  formValues: PropTypes.shape(),
};

static defaultProps = {
  formValues: {},
};

// componentWillMont instead of constructor for this.setState to be available, matching componentWillReceiveProps call
componentWillMount() {
  // I could pass `this.props` here but that would lead to the same eslint error
  this.setDisabledSteps();
}

componentWillReceiveProps(nextProps) {
  this.setDisabledSteps(nextProps);
}

// The bug is here, object destructuring in parmeters
setDisabledSteps({ formValues } = this.props) {
  const disabledSteps = [];
  [...]
  this.setState({ disabledSteps });
}

The reason I think this should be something allowed is that this is explicitly this.props that is destructured as default value.

I realize that this is quite the same case as above. But I don't get why this would be bad design.

[ljharb]

Because sometimes that function will receive props, and sometimes it will be another object. There’s no way for the reader - or a linter - to know for sure that the props object goes down that codepath.

Instead, always destructure the props object unconditionally where you need it.

[misterbridge]

Is there really no way the linter can read { formValues } = this.props within a method parameters ?
(I have no idea how linters works)

Because here it's one key but I have an other project where there was like 15 of them needed for the function call. And "your" solution would then be:

const {a,b,c,d,e,f,...} = this.props
functionCall({a,b,c,d,e,f,...});

Which is kinda messy ... :/

[ljharb]

It can read it, but what it means is that the value of the object will only be this.props when the function is called with the first argument as missing or undefined - and “how the function is called from everywhere in your program” isn’t something a linter or a human can determine from one file :-)

[misterbridge]

Sure I totally understand the fact that we can't know if it will be called with no argument. But I mean, "argument could be this.props _because it's the default value_" should be enought for linter to say ok it might be it, let's not raise a blocking error.

[ljharb]

I don’t think it is. The goal is to ensure that, if your code paths are executed, the props are used. with your code, i have no idea if the prop name is a prop (and been validated) or a random object property.

[misterbridge]

If some function call is within conditions, code paths might not be executed. (Never, or just not in some configuration cases)
If a function is exported but never used, that's dead code.
But none of those will raise errors in the linter, because the linter cannot check them and so, have to be optimistic.
There is a few cases when a linter can check for dead code (code after return, etc.) but when it can't, it should be as optimistic as he can: here, when looking at that code, it can see the obvious possibility of satisfying the rule, and IMO, so should it.

But well I understand that this might be only my opinion. A linter is there to help, not to bridle. _(That might not be the right word)_

[ljharb]

The linter is here to actively obstruct potential bugs - if there’s any chance something is a bug, it should not be merged as-is.

[misterbridge]

Well, to me linters are to avoid messy code (where here, it's doing the opposite) and testing is to avoid bugs but well... I guess that's again my opinion.
Thanks for your responses.