-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add rule to forbid className and style being passed to Components #703
Conversation
|
||
```js | ||
... | ||
"forbid-component-props": [<enabled>, { "forbid": [<string>] }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this not need a "react/" prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. I was copying how it is in other rules documentation though so we should probably fix them all at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
// Rule Definition | ||
// ------------------------------------------------------------------------------ | ||
|
||
module.exports = function(context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the new eslint rule format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up! This PR has been updated.
b8ed8f7
to
5a0444d
Compare
|
||
### `forbid` | ||
|
||
An array of strings, with the names of props that are forbidden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i want to forbid certain props on Components, but not forbid className/style, how do I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would set the forbid
option to include the props that you want to forbid and not include className/style. This is identical to forbid-prop-types.
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-prop-types.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to explicitly note that the default value of this option is ['className', 'style']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done!
Passing `className` or `style` to your Components is a source of much hidden complexity that can be solved either by using a wrapper to add styling or by adding props for specific styling. This rule aims at preventing these props from being passed to your Components, to help minimize hidden complexity. The original issue proposed having this rule check propTypes. I considered that route, but I worry that it won't be as effective for people who do not have every prop used by the component listed in the propTypes. Additionally, propTypes seem to have waning interest from the React team in favor of Flow annotations, so I think in the long run checking for this at the callsite will be more stable. More information can be found by reading: https://medium.com/brigade-engineering/don-t-pass-css-classes-between-components-e9f7ab192785 Also: https://twitter.com/sebmarkbage/status/598971268403073024 Fixes jsx-eslint#314
I updated this via the codemod from eslint-transforms. [I ran into some errors getting eslint-transforms to run correctly as described][0], but I was able to find a workaround by running the jscodeshift command directly. [0]: eslint/eslint-transforms#2 (comment) This is a completely automated change.
Listing the defaults in the documentation for these rules makes it clearer how they work.
5a0444d
to
67919e4
Compare
@yannickcr @ljharb anything else you'd like to see on this before we merge it in? |
I plan to merge this soon unless I hear otherwise. |
This rule is nice, but what if we are required by default to pass the <Modal
className="Modal__Bootstrap modal-dialog"
onRequestClose={this.handleCloseModal}
isOpen> |
Per the info in the OP, you'd simply not use the rule in that case - the idea is that passing a |
Thanks for your reply. In order to not use it, I have to disable the rule, which in turn may be contra-productive, as I can have in the same function or file other components which should indeed not receive the I propose a different approach. What about introducing an As in my example, Well, yes this approach has a down-side as I may write in the future components with that name, but I think is less likely. |
Passing
className
orstyle
to your Components is a source of muchhidden complexity that can be solved either by using a wrapper to add
styling or by adding props for specific styling. This rule aims at
preventing these props from being passed to your Components, to help
minimize hidden complexity.
The original issue proposed having this rule check propTypes. I
considered that route, but I worry that it won't be as effective for
people who do not have every prop used by the component listed in the
propTypes. Additionally, propTypes seem to have waning interest from the
React team in favor of Flow annotations, so I think in the long run
checking for this at the callsite will be more stable.
More information can be found by reading:
https://medium.com/brigade-engineering/don-t-pass-css-classes-between-components-e9f7ab192785
Also:
https://twitter.com/sebmarkbage/status/598971268403073024
Fixes #314
cc: @janpaul123 @sebmarkbage