[no-unused-prop-types] False warning when used in external function

[tleunen]

When a prop is not directly use in the class, but outside of it in an external function, the linter shows a warning.

Here's an example:

const propTypes = {
    testProp: PropTypes.string,
    content: PropTypes.string,
};

function initState(props) {
    return {
        className: `yolo-${props.testProp}`,
    };
}

export default class Test extends React.PureComponent {
    constructor(props) {
        super(props);
        this.state = initState(props);
    }

    render() {
        return <div className={this.state.className}>{this.props.content}</div>;
    }

}
Test.propTypes = propTypes;

[tleunen]

It also sometimes happen when it's used in an instance function. Will try to get a small example

[EvHaus]

Before I look into fixing this, out of curiosity, why are you using this pattern for PropType declaration? Why a separate propTypes constant? It's only used in once place.

I worry about how deep we'll have to scan for props variable re-declarations to properly account for this. For example, you could also theoretically write this code:

const propTypes = {something: PropTypes.string);
const nestedPropTypes = propTypes;
MyComponent.propTypes = nestedPropTypes;

Checking for this would require making several hops across multiple variables to detect where the prop types are actually defined.

[tleunen]

I like keeping my proptypes at the top of the file

[ljharb]

@EvNaverniouk this is how all of our propTypes are defined at airbnb, and in fact is in our styleguide, because propTypes should be at the top of the file since they're important for documentation.

[EvHaus]

Fair enough. I'll see what can be done here.

[victor-homyakov]

The same for nested function. In this example

const Comp = (props) => {
    const loadTerm = (type, projectId) => props.termProcessor.getTerm(type, projectId);

    const terms = props.terms
        .map(({type, projectId}) => loadTerm(type, projectId))
        .join(' ');

    return <span>{terms}</span>;
};

Comp.propTypes = {
    termProcessor: React.PropTypes.object.isRequired,
    terms: React.PropTypes.arrayOf(React.PropTypes.shape({
        type: React.PropTypes.string.isRequired,
        projectId: React.PropTypes.number
    })).isRequired
};

termProcessor is marked as unused, while in this example not:

const Comp = (props) => {
    const loadTerm = (type, projectId) => props.termProcessor.getTerm(type, projectId);

    const terms = props.terms
        .map(loadTerm)
        .join(' ');

    return <span>{terms}</span>;
};

Comp.propTypes = {
    termProcessor: React.PropTypes.object.isRequired,
    terms: React.PropTypes.arrayOf(React.PropTypes.shape({
        type: React.PropTypes.string.isRequired,
        projectId: React.PropTypes.number
    })).isRequired
};

[joshjg]

Similarly, here url is marked as unused.

class PhotoList extends React.Component {
  static propTypes = {
    photos: React.PropTypes.arrayOf(React.PropTypes.shape({
      id: React.PropTypes.number,
      url: React.PropTypes.string,
      caption: React.PropTypes.string,
    })),
  }

  render = () => (
    <div>
      {this.props.photos.map(photo => <div>{photo.url}</div>)}
    </div>
  )
}

[larsvinter]

And here, "id" and "onSelectIndex" is marked as unused.

import React from 'react';
import styles from './styles.css';

function PlaylistItem(props) {
  const onClick = () => {
    props.onSelectIndex(props.id);
  };

  const className = () => {
    if (props.loadedA) return styles.playlistItemLoadedA;
    if (props.loadedB) return styles.playlistItemLoadedB;
    return styles.playlistItem;
  };

  return (
    <div className={className()} onMouseDown={onClick}>
      <div className={styles.time}>{props.item.time}</div>
      <img alt="" src={props.item.coverUrl} className={styles.coverArt} />
      <div className={styles.infoBox}>
        <div className={styles.artist}>{props.item.artist}</div>
        <div className={styles.title}>{props.item.title} { props.item.version ? <i className={styles.version}>({props.item.version})</i> : null }</div>
      </div>
    </div>
  );
}

PlaylistItem.propTypes = {
  item: React.PropTypes.object,
  loadedA: React.PropTypes.bool,
  loadedB: React.PropTypes.bool,
  id: React.PropTypes.number,
  onSelectIndex: React.PropTypes.func,
};

export default PlaylistItem;

[tleunen]

Any update on this issue? @yannickcr Do you think there's an easy fix that could be done, or it's actually quite hard to know when a prop is used in a function outside of the component?

[AoDev]

Hello, Could we allow an option like: skipWithExternalFunction: true

Would pass:

static getSomeValue (props) {
  return //
}

render () {
  const someValue = MyClass.getSomeValue(this.props)
  // fine because there is function where the argument is the entire prop object
}

MyClass.propTypes = {
  prop1: React.PropTypes.number,
  prop2: React.PropTypes.bool,
  ...
};

Would not pass:

render () {
  return <p>{this.props.prop1}</p>
  // fail because prop2 isn't used.
}

MyClass.propTypes = {
  prop1: React.PropTypes.number,
  prop2: React.PropTypes.bool,
  ...
};

Caveat:

I don't know how far the code can be analysed. The only issue with this approach is that it might
have false negative. The external function might not be consuming all the props. But the point is
having some compromise until the "perfect" solution comes in.

[ljharb]

@AoDev it can't be analyzed across files; separately, passing the props object around is a huge antipattern. The proper solution is to destructure everything you need, and pass it in to your functions explicitly.

[AoDev]

"huge anti pattern" feels pretty exaggerated. I don't want to destructure and recreate possibly thousands of objects just because I have a static helper function created for the sake of code clarity. I could just move everything in the render function and it would be super bloated.

[ljharb]

@AoDev how many props does getSomeValue really need to use? You can also pass the props you need as normal arguments, instead of having to pass an object.

[AoDev]

@ljharb
Can be up to 5 - 6 arguments.
Note that this does not happen often. It's rare that such helper functions are needed. But the goal is to avoid disabling this eslint rule or making some work-around.

This is only for sake of code clarity in some particular case.
It is more readable and I wanted to avoid caring about the order of the arguments.

A real code example in one component I have, is for a scrollable listview:
(render is more complex in actual code)

static getStickAtBottomStyle({ headerIndex, height, scrollbarWidth, headersCount }) {
  return {
    position: 'absolute',
    bottom: `${(headersCount - 1 - headerIndex) * height}px`,
    width: `calc(100% - ${scrollbarWidth}px)`
  };
}

...

render () {
  ...
  {stickyState.isStickyAtBottom &&
    <div
      style={Header.getStickAtBottomStyle(this.props)}
      className="listview__header">
      {this.props.children}
    </div>
  }
  ...
}

Reasons for splitting in a function:

• can be unit tested easily
• clearer to read than putting the computations in render.

I agree with you that I could pass the arguments in order but sometimes there are bit too many.
On the other hand, these examples are almost the same ones in the docs about SFC. That's how I reached this issue in the first place ;)

[AoDev]

Note that I am trying to use the props in a static method of the same component class. Not some function in another module and / or another file.

[golopot]

Is this issue still accepted? It appears that this case, passing entire props as an argument, is an anti-pattern and is hard to implement, so I am inclined to be -1 on this. There are rejected duplicates #1135 #2155 .

[ljharb]

@golopot the OP seems to be about both that and having propTypes defined as a variable and then later assigned. Does the latter already work?

[golopot]

Yes that works 712244f