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

Document that rules dealing with prop types recognize static types, too #2546

Merged
merged 1 commit into from May 12, 2020

Conversation

@silvenon
Copy link
Contributor

@silvenon silvenon commented Jan 18, 2020

In a separate commit I cleaned up examples to be a bit more modern, preferring function components, so they are easier to read, then documented that this rule isn't only for PropTypes, but that it accepts static types, too.

@silvenon
Copy link
Contributor Author

@silvenon silvenon commented Jan 18, 2020

I realized it would be a good idea to document the same for other rules like require-default-props, but I'll first wait to see if you agree with this direction.

@ljharb ljharb force-pushed the silvenon:prop-types/static-types branch from 1ed251f to 3ef1208 Jan 28, 2020
Copy link
Collaborator

@ljharb ljharb left a comment

Happy with the overall direction.


It can warn other developers if they make a mistake while reusing the component with improper data type.
You can provide types either in runtime types using [PropTypes] or statically

This comment has been minimized.

@ljharb

ljharb Jan 28, 2020
Collaborator

let's reword this so it doesn't implicitly discourage using both - even when using static types, using PropTypes is recommended, because they can check many more things than static types can.

This comment has been minimized.

@silvenon

silvenon Jan 30, 2020
Author Contributor

Ok, I decided to simply use "and/or" instead of "or", that sounds fine to me. Let me know if you want something different.

@silvenon silvenon force-pushed the silvenon:prop-types/static-types branch from 3ef1208 to 9a4baf6 Jan 30, 2020
Copy link
Contributor Author

@silvenon silvenon left a comment

Ok, I think I found all the rules which recognize static types and I added a message to make this clear. Let me know if you don't like the blockquote formatting, one alternative could be to use horizontal rules, but that would maybe be too dramatic.

I also tried to use more agnostic language instead of "propTypes", and I sprinkled a few examples with static types.

@silvenon silvenon requested a review from ljharb Jan 31, 2020
@silvenon silvenon changed the title Document that prop-types rule recognizes static types, too Document that rules dealing with prop types recognize static types, too Jan 31, 2020
return <div>Hello {name}</div>;
// 'name' type is missing in props validation

This comment has been minimized.

@golopot

golopot Jan 31, 2020
Contributor

I tested this and it seems that currently rule prop-types does not report on props with typescript type annotation, can you confirm or refute this?

Off topic: In my opinion rule prop-types should not report on props with ts/flow type annotation, since type-checker already excelled in pointing out usage of undeclared properties and that eslint-plugin-react also reporting the same thing just adds noise.

This comment has been minimized.

@ljharb

ljharb Feb 1, 2020
Collaborator

I only think you're right if the propType exactly matches the type annotation - and since propTypes are more powerful than static types in many ways, this will often not be the case.

These examples use `createReactClass` a lot, which is legacy at this
point. I'm modifying examples to primarily use function components
because those are the ones people are using most often today. Other ways
of defining a component are just here to show that this rule recognizes
them, too.

Also, document that rules like `prop-types` and `require-default-props`
recognize static types, too
@ljharb ljharb force-pushed the silvenon:prop-types/static-types branch from 9a4baf6 to c481a26 May 12, 2020
@ljharb
ljharb approved these changes May 12, 2020
@ljharb ljharb merged commit c481a26 into yannickcr:master May 12, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 97.621%
Details
@silvenon
Copy link
Contributor Author

@silvenon silvenon commented Jun 7, 2020

I apologize for not finishing this PR 😕 I hope you managed to fix problematic parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants