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/prop-types does not validate missing propTypes for functions in version 7.11.1 #1958

Closed
blackbird91 opened this issue Aug 27, 2018 · 30 comments

Comments

@blackbird91
Copy link

blackbird91 commented Aug 27, 2018

The following patterns are considered warnings:

var Hello = createReactClass({
  render: function() {
    return <div>Hello {this.props.name}</div>;
  }
});

var Hello = createReactClass({
  propTypes: {
    firstname: PropTypes.string.isRequired
  },
  render: function() {
    return <div>Hello {this.props.firstname} {this.props.lastname}</div>; // lastname type is not defined in propTypes
  }
});

function Hello({ name }) {
  return <div>Hello {name}</div>;
}


This rule is not working in the latest version (7.11.1).
Missing propTypes for functions are not marked anymore. This was working fine on 7.10.0

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

cc @alexzherdev

@alexzherdev
Copy link
Contributor

@blackbird91 do you mean propTypes for functional components? Those are covered with tests pretty well, would you mind posting the code for the component that is having issues?

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

@alexzherdev what about createReactClass components tho?

@alexzherdev
Copy link
Contributor

createReactClass is covered with 30+ test cases. Hard to imagine what could have gone wrong without a repro.

@blackbird91
Copy link
Author

blackbird91 commented Aug 28, 2018

That's my example where props are not validated:

export class TestComponent extends Component {
    static get propTypes() {
        return {
            object: PropTypes.object,
            }
    }

    constructor(props) {
        super(props);
    }

    componentDidMount() {
        const { func } = this.props;

        func(); //does not throw missing props validation
    }

    render(){
         return <span/>;
    }     
}

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

@blackbird91 hm, the rule might not be covering misusing a static getter in that fashion; if you make it TestComponent.propTypes = { object: PropTypes.object }, does it warn as expected?

@blackbird91
Copy link
Author

It was working just fine in previous versions. Also, If I'm missing propTypes for other types of objects I get a validation message

@alexzherdev
Copy link
Contributor

This exact code is reporting a missing prop as a test case.
Is this actually the exact way the prop is being used? If this is simplified, can you post the original?

const { func } = this.props;
func();

@blackbird91
Copy link
Author

blackbird91 commented Aug 29, 2018

Capture @ version 7.11.1
capture version7 11 1

Capture @ version 7.10.0
capture version7 10 0

@ljharb
Copy link
Member

ljharb commented Aug 29, 2018

@blackbird91 please provide text, not screenshots - screenshots of code aren't nearly as helpful :-/

@blackbird91
Copy link
Author

I can't provide blocks of code, the small example I've posted before is very similar with the one I encounter in my project. I just wanted to show the difference between the 2 versions with these screen captures

@bbthorntz
Copy link

Also having the same issue. It seems to be related to components which spread props. A minimal example, for me, is the following:

Working (has a linting error):

class Component extends React.PureComponent {
    render() {
        this.props.doesNotExist(); // `react/no-props` linting error
        return <div />;
    }
}

Not working (has no linting error):

class Component extends React.PureComponent {
    render() {
        this.props.doesNotExist(); // No linting error
        return <div {...this.props} />;
    }
}

@alexzherdev
Copy link
Contributor

@blackbird91 can you also post your render method?
@bbthorntz this code should warn in the latest version

@swrobel
Copy link

swrobel commented Sep 9, 2018

A simple case that was caught in 7.10.0 & not caught by 7.11.1

import React from 'react';

export default class TestComponent extends React.Component {
  render() {
    const { prop, ...rest } = this.props;
    return <p {...rest}>{prop}</p>;
  }
}

7.10.0 output:

  5:13  error  'prop' is missing in props validation  react/prop-types

7.11.1 output shows no errors.

@asbjornh
Copy link

I'm seeing the same (or same-ish) issue with 7.11.1. The following components have no linting messages:

const Component = ({ prop }) => <div {...prop} />;
class Component extends React.Component {
  render() {
    return <div {...this.props.prop} />;
  }
}

These ones, however, do generate warnings about missing propTypes validation:

const Component = ({ prop }) => <div prop={prop} />;
class Component extends React.Component {
  render() {
    return <div prop={this.props.prop} />;
  }
}

Might be related to destructuring and/or spreading?

@alexzherdev
Copy link
Contributor

alexzherdev commented Sep 13, 2018

Interesting. I can't reproduce neither of the above examples with spreads (posted in #1958 (comment) by @asbjornh) in a test case.
Can't reproduce example from #1958 (comment) either.

@alexzherdev
Copy link
Contributor

@bbthorntz are you on the latest version? I think this should work in 7.11.1. (#1958 (comment))

@bbthorntz
Copy link

@alexzherdev yeah, version 7.11.1.

@alexzherdev
Copy link
Contributor

alexzherdev commented Sep 13, 2018

@bbthorntz I specifically worked on spread props not disabling validation of other props that are used individually. If yours is a reduced example, would you be able to post the original code? Or are you able to actually make that reduced version fail?

@swrobel
Copy link

swrobel commented Sep 13, 2018

Can't reproduce example from #1958 (comment) either.

The key seems to be actually using ...rest in the return value, as far as I can tell. 7.11.1 will complain if it's just prop that's used.

@asbjornh
Copy link

I'm able to reproduce example from #1958 (comment) (and the ones from my previous comment #1958 (comment)) with the following config:

{
  "plugins": ["react"],
  "parserOptions": {
    "ecmaVersion": 2018,
    "ecmaFeatures": {
      "jsx": true
    },
    "sourceType": "module"
  },
  "rules": {
    "react/prop-types": 1
  }
}

From yarn.lock:

eslint-plugin-react@^7.11.1:
  version "7.11.1"
  resolved "https://registry.yarnpkg.com/eslint-plugin-react/-/eslint-plugin-react-7.11.1.tgz#c01a7af6f17519457d6116aa94fc6d2ccad5443c"

eslint@^5.5.0:
  version "5.5.0"
  resolved "https://registry.yarnpkg.com/eslint/-/eslint-5.5.0.tgz#8557fcceab5141a8197da9ffd9904f89f64425c6"

If I remove the spready bits from @swrobel's component I get the expected missing propTypes message.

@alexzherdev
Copy link
Contributor

Ah, sorry for the confusion. All these issues were fixed in #1939, but that PR landed after 7.11.1. So you should expect them to get fixed in the next release.
Except for the original one posted by @blackbird91, that one I still can't figure out. I'd need to see the full code, including the render method.

@bbthorntz
Copy link

Thanks @alexzherdev! Looking forward to the fix.

@patrickvienneau
Copy link

Very excited for this fix, impatiently waiting for 7.11.2 for this fix to be released. In the meantime, do we know what the last working version for this was?

@benwill
Copy link

benwill commented Nov 1, 2018

I dropped back to 7.10.0 and that works correctly

@bdwain
Copy link

bdwain commented Nov 19, 2018

any plans to do a release soon @ljharb ? It's been a while and it would be great to not hard code the version anymore.

@ljharb
Copy link
Member

ljharb commented Nov 19, 2018

@bdwain see #2046.

@bdwain
Copy link

bdwain commented Nov 19, 2018

got it. sorry to bother. just saw it had been a long time. Thanks for all of the work you put in.

@ljharb
Copy link
Member

ljharb commented Jan 2, 2019

v7.12.1 is released; closing. Please file a new issue if there's still problems :-)

@ljharb ljharb closed this as completed Jan 2, 2019
@patrickvienneau
Copy link

Thanks for the follow-up @ljharb!

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

No branches or pull requests

9 participants