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

react/no-unused-prop-types false positives when passing props as an argument #2155

Closed
barakyosi opened this issue Feb 9, 2019 · 8 comments
Closed
Labels

Comments

@barakyosi
Copy link

In this scenario something is marked as unused although it is used.

class Hello extends Component {
  constructor (props) {
    super(props);
    this.doSomething(this.props);
  }
  doSomething(props) {
    if (props.something) {
      return true;
    }
  }
}
Hello.propTypes = {
  something: PropTypes.bool
}

It's similar to #855, but here the code is part of the class.

@ljharb
Copy link
Member

ljharb commented Feb 9, 2019

This isn't really something we can statically analyze - in general, you shouldn't be passing around the entire props (or state) object. If you destructure props in the constructor, and pass something in explicitly, it should work fine.

@barakyosi
Copy link
Author

I thought about detecting method calls with props argument, then look inside the method body and check usage.
Started to work on it but wasn't sure how to get the method body from the call expression.
Would be happy to know whether this check can be done (and if not what's the limitation)

@ljharb
Copy link
Member

ljharb commented Feb 9, 2019

In your specific simple case, it could certainly be done - but, anyone from outside the file could call doSomething with an object that's not the props - so it's simply impossible to statically know it for all circumstances, only for a subset of usage patterns.

I don't think it's worth the complexity to try to target only some of the patterns, when the entire thing can be avoided by not having the antipattern of passing the props object around.

@barakyosi
Copy link
Author

Sometimes passing all props makes it easier to follow. Especially when you need more than one prop and need to call it in different lifecycle methods.
I understand it's impossible to catch all scenarios. Would love to see a solution covering this one :)

@ljharb
Copy link
Member

ljharb commented Feb 9, 2019

I'm sure it's subjective, but I would argue that in that case, you'd want to do this:

const { a, b, c } = props;
doSomething({ a, b, c });

You can still pass an object, but it's not the actual props object, it's an explicit object you created.

@barakyosi
Copy link
Author

I agree your solution is better for this scenario, but if you need it in more than one place (in constructor & CWRP), or use many props inside doSomething - when adding a new prop you might forget to add it everywhere.

@ljharb
Copy link
Member

ljharb commented Feb 10, 2019

I see that as a feature, not a bug, since if you do forget, your tests will fail :-)

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

This seems answered - don't pass the props or state objects anywhere.

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

No branches or pull requests

2 participants