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

Enhance forbid-prop-types to check for shape({}) #1673

Closed
jeromecovington opened this issue Feb 2, 2018 · 10 comments · May be fixed by #2163
Closed

Enhance forbid-prop-types to check for shape({}) #1673

jeromecovington opened this issue Feb 2, 2018 · 10 comments · May be fixed by #2163

Comments

@jeromecovington
Copy link

Does it make sense to enhance the forbid-prop-types rule to check for cases using shape that do not actually add clarity to the code, for example shape({}).

From https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-prop-types.md

By default this rule prevents vague prop types with more specific alternatives available

And:

This rule encourages prop types that more specifically document their usage.

When using for example shape({}) we are not more specifically documenting prop usage.

@jeromecovington
Copy link
Author

Similarly it might make sense to check for arrayOf().

@ljharb
Copy link
Member

ljharb commented Feb 2, 2018

That's a very specific pattern; shape({}) is certainly only trivially better than object; same with arrayOf() vs array - but I don't think that there's really a way to generically configure that in this rule.

@jeromecovington
Copy link
Author

Yeah, I can't much argue for whether shape({}) (empty) is demonstrably better than object, only that shape({}) without any further definition of internal props doesn't actually more specifically document anything, which seemed to be the underlying goal of the forbid-prop-types. If nobody else thinks that goal is relevant, or if I am needlessly splitting hairs, I certainly won't be offended if this issue gets closed.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2018

If we can come up with a way to generically configure "propTypes that are functions that must take a non-empty argument", that'd be great.

I'm curious tho - are you actually seeing people using shape({}) or arrayOf() as a way to bypass these restrictions?

@alexlrobertson
Copy link

I use shape({}) frequently for components that consume an object and pass it to a child. The component doesn't care about the shape of the object, but the child does. Another use case would be style objects as the component doesn't need to care about the style shape.

I'm all for configurable style though.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2018

In that case, I'd recommend exporting the shape object from the child and using it on the parent, so that you get the error sooner :-)

@Palid
Copy link

Palid commented Mar 8, 2018

+1, I've seen this used a lot where developers are just being lazy and don't want to document the type.

@amankkg
Copy link

amankkg commented Apr 9, 2018

+1, I've seen this used a lot where I am just being lazy and don't want to document the type.
😅

@alexkrautmann
Copy link

I definitely would like for this to be configurable. I can't think of a good reason to do PropTypes.shape().

@ljharb
Copy link
Member

ljharb commented Feb 4, 2022

I'm still not really sure how we'd configure this. Happy to reopen if someone has a concrete idea.

@ljharb ljharb closed this as completed Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants