jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9k stars 2.77k forks source link

`react/no-unused-prop-types` fails when omitted once #1233

Closed feimosi closed 7 years ago

feimosi commented 7 years ago

In the following example I'm getting a false positive. Probably because I've used omit.

'onMeasure' PropType is defined but prop is never used
react/no-unused-prop-types
<Measure
  onMeasure={ (dimensions) => {
    this.props.onMeasure(dimensions);
  } }
>
  <ComposedComponent { ...omit(this.props, 'onMeasure') } { ...this.state }>
    { this.props.children }
  </ComposedComponent>
</Measure>
ljharb commented 7 years ago

omit shouldn't matter, it's being referenced in your onMeasure handler.

This should be fixed; however, I'd recommend destructuring all your prop values out at the top of render, which would work around this problem.

(also, you forgot to omit children)

feimosi commented 7 years ago

Thanks 👍

SuEric commented 7 years ago

I've the same problem and I find difficult to destructing my prop values at the top of render;

Error:

error  'dateFormatter' PropType is defined but prop is never used     react/no-unused-prop-types
error  'showAnswersBadge' PropType is defined but prop is never used  react/no-unused-prop-types

Code where I use my prop

<p style={{ fontSize: '14px', display: 'inline-block' }} >{props.dateFormatter(question.updatedAt)}</p>
{
  props.showAnswersBadge &&
  <Badge
    style={{ display: 'inline-block', padding: 0 }}
    badgeContent={question.answersCount}
    primary
  >
    <IconButton iconClassName="material-icons">question_answer</IconButton>
  </Badge>
}
{
  props.showViewsBadge &&
  <Badge
    style={{ display: 'inline-block', padding: '0' }}
    badgeContent={question.viewsCount}
    primary
  >
    <IconButton iconClassName="material-icons">visibility</IconButton>
  </Badge>
}

PropTypes declaration

QuestionList.propTypes = {
  questions: React.PropTypes.array,
  dateFormatter: React.PropTypes.func.isRequired,
  showAnswersBadge: React.PropTypes.bool,
  showViewsBadge: React.PropTypes.bool,
};

I cannot change structure since my dateFormatter prop & <Badge> after props.showAnswersBadge conditional need access to question which is obtained from questions array prop after doing questions.map

Hope any of you can help of how to change the structure or if this will be fixed any soon. I think this is related with #1234 ? which is already fixed?

jseminck commented 7 years ago

@SuEric could you try to run your code against master? Then we can see if this case is fixed already or not. Judging by the code, I cannot be completely sure...

You can do it this way: npm install git://github.com/yannickcr/eslint-plugin-react.git

SuEric commented 7 years ago

I tried but I've problems with my webpack config after doing upgrade. Give me some time to solve it, ty

SuEric commented 7 years ago

@jseminck I used git://github.com/yannickcr/eslint-plugin-react.git and still have same problem.

  104:18  error  'dateFormatter' PropType is defined but prop is never used     react/no-unused-prop-types
  105:21  error  'showAnswersBadge' PropType is defined but prop is never used  react/no-unused-prop-types
jseminck commented 7 years ago

Ok - I'll try to have a look and see if I can reproduce.

More on-topic. I tried to use _.pick() to reduce some code for prop passing. Turns out this is also not being validated correctly. Not sure if it should be supported...

E.g:

export default class Create extends React.Component {
    static propTypes = {
        currentSubmission: PropTypes.object.isRequired,
        addLocation: PropTypes.func.isRequired,
        cancelSubmission: PropTypes.func.isRequired,
        submissionFieldChange: PropTypes.func.isRequired,
    };

    render() {
        return (
            <div className="row col-md-8">
                <StaticForm {..._.pick(this.props, "currentSubmission", "submissionFieldChange")} />
                <LocationsForm {..._.pick(this.props, "addLocation", "currentSubmission")} />
                <CancelButton {..._.pick(this.props, "cancelSubmission")} />
            </div>
        );
    }
}
  16:28  error  'currentSubmission' PropType is defined but prop is never used      react/no-unused-prop-types
  18:22  error  'addLocation' PropType is defined but prop is never used            react/no-unused-prop-types
  19:27  error  'cancelSubmission' PropType is defined but prop is never used       react/no-unused-prop-types
  20:32  error  'submissionFieldChange' PropType is defined but prop is never used  react/no-unused-prop-types
ljharb commented 7 years ago

There's no way for any linter to know what you're dynamically picking - you should instead destructure those out like const { currentSubmission, addLocation, submissionFieldChange, cancelSubmission } = this.props;, and then it'll be able to detect it.

SuEric commented 7 years ago

Maybe this is happening because that code is under a array.map() call?

ljharb commented 7 years ago

No, it's because someRandomPickFunction(this.props, 'any prop name') is impossible to statically determine as accessing "any prop name" on this.props. That's metaprogramming, and metaprogramming obstructs linting.

The proper solution is to statically reference all prop names out of this.props, which then enables many kinds of static analysis to be done on it.

SuEric commented 7 years ago

I'm speaking about my issue. Is not dynamically called. Can you tell me what might be wrong? See my code comments above

ljharb commented 7 years ago

@SuEric ah, sorry. that doesn't seem to be the entire code; could you provide the entire render method?

SuEric commented 7 years ago

Hey @ljharb ;

I see problem is when accessing this.props.propName inside a map.. I was calling props.dateFormatter & props.showAnswersBadge inside this props.questions.map((question) => {}). In order to dissapear this Eslint error; I assigned props.dateFormatter & props.showAnswersBadge to some const variables.

Is this expected behavior??

Also, PLEASE NOTE THE FOLLOWING: I'm not getting errors with props.showViewsBadge which is being used in the same way as props.showAnswersBadge

Here is the code without any Eslint ERROR.

function QuestionList(props) {
  const dateFormatter = props.dateFormatter;
  const showAnswersBadge = props.showAnswersBadge;
  ...
  ... 
  return (
    <div>
      <h1>{props.header}</h1>
      <Paper zDepth={1} style={{ padding: '1em' }}>
        {props.form}
        <List>
          <Subheader>{ <FormattedMessage {...messages.questionList.header} /> }</Subheader>
          {props.questions.map((question) => (
            <Link
              style={{ textDecoration: 'none' }}
              key={question._id} // eslint-disable-line no-underscore-dangle
              to={`/main/questions/${question._id}`} // eslint-disable-line no-underscore-dangle
            >
              <div>
                <ListItem
                  leftAvatar={avatar}
                >
                  ...
                     ...
                        ...
                          ...
                          {question.tags.map((tag) => (
                            <Chip
                              key={tag}
                              style={{ display: 'inline-block', marginRight: '0.5em' }}
                            >
                              {tag}
                            </Chip>
                          ))}
                        </div>
                      </div>
                    </div>
                    <div>
                      <p style={{ fontSize: '14px', display: 'inline-block' }} >{dateFormatter(question.updatedAt)}</p>
                      {
                        showAnswersBadge &&
                        <Badge
                          style={{ display: 'inline-block', padding: 0 }}
                          badgeContent={question.answersCount}
                          primary
                        >
                          <IconButton iconClassName="material-icons">question_answer</IconButton>
                        </Badge>
                      }
                      {
                        props.showViewsBadge &&
                        <Badge
                          style={{ display: 'inline-block', padding: '0' }}
                          badgeContent={question.viewsCount}
                          primary
                        >
                          <IconButton iconClassName="material-icons">visibility</IconButton>
                        </Badge>
                      }
                    </div>
                  </div>
                </ListItem>
                <Divider inset />
              </div>
            </Link>
          ))}
        </List>
      </Paper>
    </div>
  );
}

QuestionList.propTypes = {
  ...
  dateFormatter: React.PropTypes.func.isRequired,
  showAnswersBadge: React.PropTypes.bool,
  showViewsBadge: React.PropTypes.bool,
  ...
};

Hope this helps, thanks!

jseminck commented 7 years ago

Thanks. I managed to reproduce... Here's a more concise example what doesn't work:

export default function QuestionList(props) {
    return (
        <div>
            {props.questions.map((question) => (
                <div key={question.id}>
                    <p>{props.dateFormatter(question.updatedAt)}</p>
                    {props.showAnswersBadge && <div>Show Answers!</div>}
                    {props.showViewsBadge && <div>Show views</div>}
                </div>
            ))}
        </div>
    );
}

QuestionList.propTypes = {
    questions: PropTypes.array.isRequired,
    dateFormatter: PropTypes.func.isRequired, // not found
    showAnswersBadge: PropTypes.bool, // not found
    showViewsBadge: PropTypes.bool,
};

If I remove {props.showViewsBadge && <div>Show views</div>} then it will detect showAnswersBadge, but not dateFormatter.

If I remove also {props.showAnswersBadge && <div>Show Answers!</div>} then it will detect dateFormatter.

I'll see what I can do about this!

Edit: Have you considered splitting up the inner Question JSX into it's own component?

ljharb commented 7 years ago

I'm not seeing why you wouldn't want to destructure everything that comes out of props in the top line of render?

SuEric commented 7 years ago

@jseminck Thanks for reproducing it.

Like I said I removed those errors by assigning my props to some const variables before the return () statement.

But was asking if this should be expected; maybe is more an enhancement than a bug? But I think it should really be able to detect its usage inside a .map(()=>{}) call; it's supposed to analyze the code, so doesn't matter if there's nothing to .map().

Have you considered splitting up the inner Question JSX into it's own component?

Even if splitting up... would still be same... need to .map(()=>{}) each item in order to render it. Is like if after this .map(()=>{}) call no props are recognized. Or am I missing something?

Regards,

ljharb commented 7 years ago

I think there's a limit how much static analysis can be done to determine where props are used in the render path; the proper solution imo is to always destructure all props and state at the top of every instance method you use them in.

SuEric commented 7 years ago

Sorry @ljharb I'm not getting this:

I think there's a limit how much static analysis can be done to determine where props are used in the render path

What's the limit problem? 😱 🤔

Sorry just for me doesn't make sense to me having to "destructure" props by setting it to a const variable each time I'll do some .map(()={}) operation.

ljharb commented 7 years ago

Maintaining a state machine in the linter rule to backtrack whenever a this.props reference is seen, and then later only warning on unseen ones, is a bit tricky. If it seems easy you are more than welcome to submit a PR that enhances the rule!

In general, though, destructuring all props and state at the top of the function makes things more readable - it places them right next to the arguments, so it's easier to reason about all the conceptual inputs to the function all in one place, and as a side benefit, all the linting rules about props suddenly give you much more value.

ljharb commented 7 years ago

Closed in #1331.