Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positives for [no-unused-prop-types] when props is assigned to a local variable #2177

Closed
laumair opened this issue Feb 26, 2019 · 6 comments · Fixed by #2737
Closed

Comments

@laumair
Copy link

laumair commented Feb 26, 2019

class App extends Component {
  static propTypes = {
    notifications: PropTypes.array.isRequired
  };

  customizeNotifications() {
    const props = this.props;

    return props.notifications.map((notification) => `foo${notification}`);
  }

  render() {
    const notifications = this.customizeNotifications();

    return (
      <View>
        {notifications.map((notification) => <Text>{notification}</Text>)}
      </View>
    );
  }

Eslint complains about notifications prop as unused.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2019

Presumably if you const { notifications } = this.props instead, it passes?

@laumair
Copy link
Author

laumair commented Feb 27, 2019

Presumably if you const { notifications } = this.props instead, it passes?

Yep, it passes on destructuring props.

@ljharb
Copy link
Member

ljharb commented Feb 27, 2019

Although I’d strongly recommend that pattern, yours should certainly work if possible.

@jzabala
Copy link
Contributor

jzabala commented Jul 13, 2020

@ljharb the optimal solution would be to handle every possible edge case but things can get out of hand very quickly. Like:

  • Passing the props as argument renderSomething(this.props)
  • Or reassigning props more that one:
var props = this.props
var moreProps = {more: true, ...props}
// ... do something with moreProps

In the case presented by this issue what would be the added value of this:

customizeNotifications() {
    const props = this.props;
    return props.notifications.map((notification) => `foo${notification}`);
  }

vs

customizeNotifications() {
    return this.props.notifications.map((notification) => `foo${notification}`);
  }

@ljharb
Copy link
Member

ljharb commented Jul 13, 2020

I don't think there's much added value - they should do const { notifications } = this.props; regardless :-) but it's nice when we can correctly analyze as much as possible.

I think eslint provides a number of utilities to make tracking the source of a variable easier?

@jzabala
Copy link
Contributor

jzabala commented Aug 2, 2020

I don't think there's much added value - they should do const { notifications } = this.props; regardless :-) but it's nice when we can correctly analyze as much as possible.

Awesome.

I think eslint provides a number of utilities to make tracking the source of a variable easier?

Yeah, you can use context.getDeclaredVariables for that. In this case though is wasn't necessary since it seems it was fixed by another PR. I created a PR with a test for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants