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

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

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.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@silvenon silvenon Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@silvenon silvenon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 January 31, 2020 00:01
@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
Copy link
Contributor

@golopot golopot Jan 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 merged commit c481a26 into jsx-eslint:master May 12, 2020
@silvenon
Copy link
Contributor Author

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants