Skip to content

Conversation

@xkr47
Copy link
Contributor

@xkr47 xkr47 commented Feb 24, 2017

According to https://facebook.github.io/react/docs/typechecking-with-proptypes.html there were flaws in the implementation and tests suggesting that you could specify proptypes like:

        'h: PropTypes.arrayOf,',
        'j: PropTypes.instanceOf,',
        'l: PropTypes.objectOf,',

when the referenced documentation actually declares some of the proptypes (arrayOf etc) expect to be called like functions with specifically typed arguments. I have updated the code to support all variants documented in the above location. It ended up being quite a major overhaul. I also updated & added tests to match, including the example configuration from the above location as one test case.

I hope you can consider this, since the flaws have previously required me to disable the rule altogether when using more advanced proptype declarations.

@xkr47
Copy link
Contributor Author

xkr47 commented Feb 24, 2017

And it also adds support for (React.)PropTypes.symbol which was previously missing.

@craigbilner
Copy link
Owner

Thanks for your work on this @xkr47

As this hasn't been touched for a while and was made for a very specific purpose I didn't have the whole pipeline setup for contribution. I've now added CI and linting.

I've also upgraded eslint and fixed up the test configs as it was very out of date. If you could rebase and get everything linted and passing that would be good, ta.

Looking at the PR. Just a couple of notes:

  • Can you update the docs to tell everyone about the stricter policy
  • Can you clean up some of the invalid code so that it's focussed on just invalid props and move into separate files as you did with yours so it's consistent?

Thanks again

@xkr47
Copy link
Contributor Author

xkr47 commented Mar 8, 2017

Sounds great, I will look into it. Thanks for considering my submission!

@xkr47
Copy link
Contributor Author

xkr47 commented Mar 8, 2017

Rebased & linted. Bullet points not done yet.

@xkr47
Copy link
Contributor Author

xkr47 commented Mar 8, 2017

Test code cleaned. Should the expected errors also be moved to separate files e.g. invalid-1.errors.js?

@xkr47
Copy link
Contributor Author

xkr47 commented Mar 8, 2017

Sorry couldn't resist to add support for contextTypes validation as well. It was too easy :)

Anyway, I hope I have made all the changes you requested now. Care to take a look?

@craigbilner craigbilner merged commit 28719e2 into craigbilner:master Mar 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants