Eslint-plugin-react: Destructured use of property is not recognized by no-unused-prop-types

[kumar303]

Given a React component like:

export default class Thing extends React.Component {
  static propTypes = {
    i18n: PropTypes.shape({
      gettext: PropTypes.func,
    }).isRequired,
  }

  render() {
    const { i18n } = this.props;

    return (
      <div>
        <span>{i18n.gettext('Some Text')}</span>
      </div>
    );
  }
}

And the following eslint rule:

"react/no-unused-prop-types": "error"

I see the following error:

/Users/kumar/tmp/eslint-scratch/index.js
  6:16  error  'i18n.gettext' PropType is defined but prop is never used  react/no-unused-prop-types

This is incorrect because the i18n property is destructured into a new constant and then the gettext() function is called. If I edit the code so that it doesn't use destructuring then the error goes away, revealing the bug.

Here is a small app that reproduces it: eslint-scratch.zip. Run:

npm install
npm run eslint

This is similar to https://github.com/yannickcr/eslint-plugin-react/issues/782 but they seem different.

[yury-dymov]

I am facing same issue with very similar use-case

const propTypes = {
  clientVersion:  PropTypes.string,
};

const defaultProps = {
  clientVersion: null,
};

class App extends Component {
  constructor(props) {
    super(props);

    this.checkClientVersion = this.checkClientVersion.bind(this);
  }

  checkClientVersion(props = this.props) {     // <-- might puzzle parser but still a valid use-case
    const { clientVersion } = props;                   // <-- used!
   <...>
  }

  render() {
    <...>
}

App.propTypes     = propTypes;
App.defaultProps  = defaultProps;

export default App;

[ljharb]

@yury-dymov in your case, it's only used if they don't pass in a props arg - can I ask what the use case for that is? It seems very un-React.

[yury-dymov]

@ljharb, well, to me it looks very common

  componentDidMount() {
    this.checkClientVersion();
  }

  componentWillReceiveProps(nextProps) {
    <...>
    this.checkClientVersion(nextProps);
  }


  checkClientVersion(props = this.props) {
    const { clientVersion } = props;
    <...>
  }

So, basically it is helper method, which is called to do whatever, when props are provided/updated. Normally it is called with no parameters but for componentWillReceiveProps() I want to pass nextProps instead of current.

[ljharb]

If it's a helper method, it shouldn't be an instance method - and then this plugin won't warn on it. Instance methods should use this in all code paths, otherwise they shouldn't be instance methods.

[tristanbbq]

Could this be related: https://github.com/yannickcr/eslint-plugin-react/pull/611 ?

[EvHaus]

I've been thinking about the best way to handle this. The only thing I can think of is to build some common conventions into the rule. ie. Any function that uses the following patterns this.props.something or props.something, or nextProps.something or prevProps.something would count towards being considered used.

It will prevent false positive warnings, but would also not catch certain cases where the object isn't actually the component props.

Thoughts?

[nkt]

This is not linting rule, it's static analysis rule. Static analyzer should be able to resolve variables instead just checking them by some "mask".
I guess this project needs magic function resolveVariable, this function can solve many issues:

// user code
const { settings: { foo } } = this.props;

// plugin
resolve('foo') -> this.props.settings.foo

// user code
const { users } = this.props;
users.map(({ id }) => ...);

// plugin
resolve('id') -> this.props.users[].id

[Andreyco]

I got false positive connected to destruction assignment.

Code preview

export default class NavigationButton extends React.Component {
  static propTypes = {
    route: PropTypes.shape({
      getBarTintColor: PropTypes.func.isRequired,
    }).isRequired,
  };

  renderTitle() {
    const { route } = this.props;
    return <Title tintColor={route.getBarTintColor()}>TITLE</Title>;
  }
}

I get this false report, despite getBarTintColor is clearly used.

'route.getBarTintColor' PropType is defined but prop is never used

[juliacochran]

I am seeing this as well when using PropTypes.arrayOf(PropTypes.shape({...}) and then mapping the prop in render

[benfletcher]

Sorry to pile on, but I believe this is one more unique example of inaccurate triggering of the when props are destructured and reassigned.

import React from 'react';

const Component = ({ children: aNode }) => (
  <div>
    {aNode}
  </div>
);

Component.defaultProps = {
  aNode: null,
};

Component.propTypes = {
  aNode: React.PropTypes.node,
};

Throws two errors:
'Children' is missing in props validation at ...
'aNode' PropType is defined but never used at ...

[Andreyco]

@numbergames
Warnings are valid, in this case.

const Component = ({ children: aNode }) => (
   // ...
);

// is equivalent of this

const Component = (props) => {
  const { children: aNode } = props;
  // ...
}

Thus, all prop validation does apply to original prop names - not aliased. Same applies to defaultProps

Correct usage would be:

import React from 'react';

const Component = ({ children: aNode }) => (
  <div>
    {aNode}
  </div>
);

Component.defaultProps = {
  children: null,
};

Component.propTypes = {
  children: React.PropTypes.node,
};

[mike-robertson]

Here's an even weirder bug that may be related to all this. I'm using flow and this keeps coming up:

  onInputChange({ target: { value: inputValue } }: { target: { value: string } }) {
  //                                                           ^^^^
  // target.value PropType is defined but prop is never used (react/no-unused-prop-types)
    this.setState({
      inputValue,
      inputWidth: Select.defaultInputWidth + (7 * inputValue.length),
    });
  }

I have no idea why this would think there are props here. It's inside a class component; it's not even related to props, I'm just destructuring an event object.

[ljharb]

@mike-robertson yours is likely a bug specific to Flow; can you file a separate issue for that?

[anshulsahni]

My code in react component

const bolusInsulin = find(props.insulins, { value: getIdFromUrl(props.bolusInsulin) }) || {};

In PropTypes

GlucoseSettings.propTypes = {
  insulinRegimen: PropTypes.string,
  bolusInsulin: PropTypes.string,
  insulins: PropTypes.array,
};

still the error reads

72:17  error  'bolusInsulin' PropType is defined but prop is never used  react/no-unused-prop-types
73:13  error  'insulins' PropType is defined but prop is never used      react/no-unused-prop-types

[ljharb]

@anshulsahni Can you share more code? Where does that find appear?

[anshulsahni]

@ljharb

this is the full component code, certain pieces of code are hidden

import React, { PropTypes } from 'react';

import pick from 'lodash/pick';
import find from 'lodash/find';

import { getIdFromUrl } from ...
import CorrectionalInsulin from ...
import InsulinRegimen from ...
import CarbCounting from ...
import LimitsEdit from ...

import '../../styles/index.scss';

const correctionalInsulinProps = [
...
];

const limitsEditProps = [
...
];

const insulinRegimenProps = [
....
];

const GlucoseSettings = (props) => {
  const renderCorrectional = () => (
    <CorrectionalInsulin
      {...pick(props, correctionalInsulinProps)}
    />
  );

  const renderCarbCounting = () => {
    const bolusInsulin = find(props.insulins, { value: getIdFromUrl(props.bolusInsulin) }) || {};
    return (
      <CarbCounting
        bolusInsulin={bolusInsulin.name}
      />
    );
  };

  return (
    <div className="glucose-settings">
      <LimitsEdit
        {...pick(props, limitsEditProps)}
      />
      <InsulinRegimen
        {...pick(props, insulinRegimenProps)}
      />
      {props.insulinRegimen.includes('carb_counting') ? renderCarbCounting() : null}
      {props.insulinRegimen.includes('mixed') ? null : renderCorrectional()}

    </div>
  );
};

GlucoseSettings.propTypes = {
  insulinRegimen: PropTypes.string,
  bolusInsulin: PropTypes.string,
  insulins: PropTypes.array,
};

GlucoseSettings.defaultProps = {
  insulinRegimen: '',
  insulins: [],
  bolusInsulin: '',
};

export default GlucoseSettings;

and the error reads

72:17  error  'bolusInsulin' PropType is defined but prop is never used  react/no-unused-prop-types
73:13  error  'insulins' PropType is defined but prop is never used      react/no-unused-prop-types

[mqklin]

I have a warning with flow too. My code:

type Props = {
  obj: {
    num: number, // < obj.num PropType is defined but prop is never used (react/no-unused-prop-types)
  },
};
...
static propTypes = {
  obj: PropTypes.shape({
    num: PropTypes.number, // < no error here
  }),
};

props: Props;
...
render() {
  const {obj: {num}} = this.props;
  return <div>{num}</div>;
}

And my solution:

type Props = {
  /* eslint-disable */
  obj: {
    num: number,
  },
  /* eslint-enable */
};

I can create a repro if it's necessary.

[robguy21]

Just adding my snippet here...

type HeaderPropTypes = {
  actions: { unAuth: () => any }, // error  'actions.unAuth' PropType is defined but prop is never used  react/no-unused-prop-types
};

const Header = ({ actions }: HeaderPropTypes): React$Element<*> => (
  <div>
      <button
        className="btn--secondary"
        onClick={
          function logout(): boolean {
            flushAuth(actions.unAuth);
            return true;
          }
        }
      >
        Log Out
      </button>
  </div>
);

const mapDispatchToProps = (dispatch: () => any): {} => ({
  actions: bindActionCreators(Actions, dispatch),
});

Following @mqklin my solution was to disable that specific rule...

type HeaderPropTypes = {
  /* eslint-disable react/no-unused-prop-types */
  actions: { unAuth: () => any },
  /* eslint-enable react/no-unused-prop-types */
};

[anshulsahni]

@ljharb any update on this ?

[ljharb]

Nope, it's marked "help wanted"

[mrm007]

@ljharb How can we help? I can take a stab at it if you point me in the right direction.

[ljharb]

@mrm007 i'd start by adding the relevant test cases to no-unused-prop-types, and then try to make them pass :-D

[mrm007]

It's this kind of snark that I came here for. Thanks!

[ljharb]

@mrm007 i realize the tone is snarky, but i'm quite serious - that's the best way to go about fixing it. I'm not actually sure beyond that what AST parsing changes you might need to make to fix the issue; otherwise i'd have made a PR myself :-)

[BertelBB]

Another one for you

componentDidMount = () => this.checkForAccount(this.props)

componentWillReceiveProps = nProps => this.checkForAccount(nProps)

checkForAccount({ account, ... }) {...}

...

AccountContainer.propTypes = {
    account: PropTypes.shape(PropTypes.shape({
        ...
    }).isRequired).isRequired,
    ...
}

[eslint] 'account' PropType is defined but prop is never used (react/no-unused-prop-types)

[ljharb]

@BertelBB I'm confused - why are your lifecycle methods instance properties instead of prototype methods? That seems wildly incorrect.

Regardless, passing around the entire props object is always going to make linting it untenable - destructure it the first time you interact with it, and pass those items around, and everything should work fine.

[BertelBB]

@ljharb I'm also confused. I'm not sure what you mean.

I'm pretty sure my lifecycle methods are not instance properties.

import React, { Component } from 'react'
...
export default class AccountContainer extends Component {
    constructor(props) {
        super(props)
        this.state = {
            someStatePassableDownChain = '' // <- Instance property ?
        }
        ...
        this.checkForAccount = this.checkForAccount.bind(this) // <- Also instance property ?
    }
    componentDidMount = () => this.checkForAccount(this.props)

    componentWillReceiveProps = nProps => this.checkForAccount(nProps)

    checkForAccount = ({ account, ... }) => {...}

    ...
}

AccountContainer.propTypes = {
    account: PropTypes.shape(PropTypes.shape({
        ...
    }).isRequired).isRequired,
    ...
}

The reason for passing the entire props object to checkForAccount, is because I am using more than one property in that function. Depending on if a specific property has changed, I need to do stuff. I would like to pass the entire object and destructure it in checkForAccount, rather than pass each property individually as a parameter in each lifecycle method. Is there something wrong with that?

[ljharb]

Because you're using class property syntax (componentDidMount = anArrowFunction) they're instance properties. You need to be using method syntax:

export default class AccountContainer extends Component {
    constructor(props) {
        …
    }
    componentDidMount() {
          this.checkForAccount(this.props);
        }

    componentWillReceiveProps(nProps) {
          this.checkForAccount(nProps);
        }

    checkForAccount({ account, ... }) {
          …
        }

    ...
}

[BertelBB]

@ljharb Ok I had no idea there were more than syntax differences.. thanks so much!

[DianaSuvorova]

@ljharb I have created a PR with (what I think) all of the valid use cases discussed in this thread.
Most of them are fixed already. I found only one reported by @robguy21, that needs to be investigated (I commented it out and will look into it). Please let me know if missed any other valid cases from this issue. Thanks!

[DianaSuvorova]

@ljharb, here is the summary of this thread.

There are 3 separate types of errors were reported/discussed here:

• PropTypes.shape officially this rule doesn't support it . By default the options for this rule is [{skipShapeProps: true}]. There are quite a few people reported test cases that uses PropTypes.shape (including the very first one) without any modification their test cases passes as I demonstrated in a PR. As soon as one sets [{skipShapeProps: false}] these cases are going to fail.

• Destructuring assignment. This works just fine. The problem is that destructuring assignment is often times used with shape type. Thus it is really easy to assume that it is a problem.

• Flow Components. This is the one I though needs to be checked but then I realized it is a flow. So I am not really sure what's this library strategy for supporting flow. For this rule I would propose to skip flow components (can be a config option) unless there is a better ideas. This can be done as a separate ticket/PR.

With that I think we can actually close this ticket. I will update my pr to format the tests correctly. But there is going to be no code change.

LMK your thoughts.

--Update---
PR is updated 😃

[ljharb]

Thanks for that summary - I'd say the flow issues should be supported, ideally, by a separate PR is fine; and the issues with shape props are known.

[leonardovillela]

The flow case is fixed? Exists other issue for it?

[djbobbydrake]

• Destructuring assignment. This works just fine. The problem is that destructuring assignment is often times used with shape type. Thus it is really easy to assume that it is a problem.

I believe there's been a regression in 7.14.3. When I destructure a proptype while in 7.14.3, it errors out for me. When I pin the version to 7.14.2, no errors.

[ljharb]

@djbobbydrake what about v7.15.1?

[djbobbydrake]

@ljharb errors out for me in 7.15.1 also

[ljharb]

Thanks for confirming; can you please file a new issue? The sooner we can get a test case the sooner we can fix it :-)