prop-types fails with destructuring

[MatthewHerbst]

The prop-types rule fails when presented with destructuring:

package.json

{
  "dependencies": {
    "eslint": "^4.6.1",
    "eslint-plugin-react": "^7.3.0",
    "proptypes": "^1.1.0",
    "react": "^15.6.1"
  }
}

.eslintrc

{
  "env": {
    "es6": true,
    "node": true
  },
  "parserOptions": {
    "sourceType": "module"
  },
  "plugins": ["eslint-plugin-react"],
  "rules": {
    "react/prop-types": 2
  }
}

Examples.jsx

import React from 'react';
import PropTypes from 'proptypes';

// FAIL: Should error that `bar` is missing in propTypes
class DestructuredDeepMissing extends React.PureComponent {
  render () {
    const {
      foo: {
        bar,
      },
    } = this.props;

    const it = bar;
  }
}
DestructuredDeepMissing.propTypes = {
  foo: PropTypes.shape({}).isRequired,
};

// Interestingly, non-deep destructuring works just fine
// PASS: Should error
class DestructuredMissing extends React.PureComponent {
  render () {
    const {
      foo,
    } = this.props;

    const it = foo;
  }
}
DestructuredMissing.propTypes = {};

Related: #296
Possibly related: #1346

Also possibly related is that no-unused-prop-types (without using Flow) fails on destructuring as well, which is behavior I was able to replicate (though there seem to be a plethora of tickets related to this, especially in the Flow context).

[ljharb]

Shape props have known bugs; do you have skipShapeProps enabled?

[MatthewHerbst]

@ljharb no - we use Relay, so basically all of our props are within a shape (viewer.x, or deeper). Enabling that option would be akin to turning this rule off for us. I know that no-unused-prop-types has issues with shapes since it mentions that in the description, but there is no such warning in the prop-types rule (though makes sense that the two rules would share some issues).

[jseminck]

We have this test case:

      code: [
        'class Hello extends React.Component {',
        '  render() {',
        '    this.props.a.b',
        '    return <div>Hello</div>;',
        '  }',
        '}',
        'Hello.propTypes = {',
        '  a: PropTypes.shape({',
        '  })',
        '};'
      ].join('\n'),
      errors: [{
        message: '\'a.b\' is missing in props validation'
      }]

Judging from that, it seems some basic shapes support exists and the example should actually be supported.

Indeed when I try the following snippet I actually get an error. But the destructuring doesn't work with the shapes...

class DestructuredDeepMissing extends React.PureComponent {
  render () {
    this.props.foo.bar;
  }
}
DestructuredDeepMissing.propTypes = {
  foo: PropTypes.shape({}).isRequired,
};

[Tokimon]

I have sort of the same error, but not with any PropTypes.shape, but with a rest a destructuring.

My example

class MyComponent extends React.Component {
  static displayName = 'MyComponent';
  static propTypes = {
    name: PropTypes.string
  }

  render() {
    const { name, title, ...rest } = this.props;
    return (
      <div {...rest}>
        <h1>{name}</h1>
      </div>
    );
  }
}

Here it fails to detect title as not defined in the propTypes.

I have narrowed it down to the spread on the JSX element: <div {...rest}>. When this is enabled the detection turns off. This is event if the spread happens in a separate context (ie. another method) like this:

class MyComponent extends React.Component {
  static displayName = 'MyComponent';
  static propTypes = {
    name: PropTypes.string
  }

  renderName() {
    const { name, ...rest } = this.props;
    return <h1 {...rest}>{name}</h1>;
  }

  render() {
    const { title } = this.props;
    return (
      <div>
        {title}
        {this.renderName()}
      </div>
    );
  }
}

In this example, if I remove the {...rest} from <h1 {...rest}> the title is correctly being marked as "not defined in prop-types".

[ljharb]

That seems like an improvement to be made.

[sstern6]

@ljharb looked into this tonight locally with latest master, it actually looks like this has been fixed in a commit between last release and today. The code above is erroring correctly. I believe this should be fixed when you publish a new version. Can you confirm?

#1958 (comment)

Thanks

[ljharb]

oh, great! Could you open a PR with these test cases, and then we can close this issue?

[sstern6]

Sure! @ljharb

[sstern6]

@ljharb #2029. When youre ready.

Let me know if you have any feedback.