static analysis of propTypes

[aw-davidson]

I've noticed some patterns that can interfere with eslint's static analysis of propTypes. For example:

Button.propTypes = {
             color: PropTypes.string,
             ...paddingPropTypes,
             caption: propTypes.string
}

Also, unpacking props from this will break static analysis: const { props } = this;

These patterns interfere with eslint telling you a prop is missing in props validation. I would like to contribute my solution for this issue.

[ljharb]

The former case should be covering color and caption, and the spread should just be props that are impossible to statically analyze.

const { props } = this definitely shouldn't break anything that uses this.props.

[alexzherdev]

I think the former case is fixed in master. I had a similar test case here https://github.com/yannickcr/eslint-plugin-react/pull/1939/files#diff-9dbe8e4f339ea9d6faa56b31c38e71bbR4871
The latter case is similar to #1764, except it's a different rule. Although, since used props detection is a common helper now, it only needs to be fixed in one place.

[aw-davidson]

Spreading propTypes was actually breaking static analysis for all props, but I might have been on an older version. I'll check later.

For const { props } = this, I wrote a rule warning against unpacking props from this. Analysis for this.props.something still will work, but does not work for props.propNotInPropTypes. And often times you will only use props and not this.props after unpacking in this way. I was seeing this pattern a lot in my codebase.

[aw-davidson]

#1764 is the same issue. This happens anytime there is a reference to props. const props = this.props would also be another case.

As regards to the other issue (spread in propTypes definition), this is breaking static analysis for all props for me. Using eslint-plugin-react v 7.11.1 and react v 15.5.4.

[ljharb]

v7.12.0 is released; is this still an issue?

[atzec]

v7.12.0 is released; is this still an issue?

Yes. We use things like:

class C extends React.Component {

    static propTypes = {
        ...B.propTypes,
        own: PropTypes.any
    }

    static defaultProps = {
        foo: "bar" // defined in B.propTypes but reported as missing from C.propTypes
    }

}

[ljharb]

That’s impossible to fix with a linter; I’d recommend spreading in both B.propTypes and B.defaultProps so there’s no confusion about where the source of truth is.

[aw-davidson]

@lagesag that looks like a different issue.

@ljharb I am stilling having this issue after updating to 7.12.0. Again, the issue is that using spread in proptypes disables static analysis:

class Button extends React.Component {

   render() {
       this.props.notDefinedInProptypes() // expect: 'notDefinedInPropTypes' is missing in props validation 
       return <div>button</div>
   }
   Button.propTypes = {
        ...paddingPropTypes,
        color: PropTypes.string
    }
}

Is this the expected behavior because any prop could be in paddingPropTypes?

[ljharb]

@aw-davidson correct, this is the intended behavior.

[aw-davidson]

@ljharb what about a rule that warns against dynamically defining propTypes?

[ljharb]

Spreading shared propTypes into a component's propTypes is not an antipattern, it's a good pattern.

At some point, linting can't catch everything, and you have to let your unit tests catch things :-)