From 986e00e3411de4d95216c11057c5f0017b9791ec Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Sun, 24 Jul 2016 10:21:31 -0700 Subject: [PATCH 1/3] Add rule to forbid className and style being passed to Components 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 #314 --- README.md | 1 + docs/rules/forbid-component-props.md | 44 +++++++ index.js | 1 + lib/rules/forbid-component-props.js | 59 +++++++++ tests/lib/rules/forbid-component-props.js | 153 ++++++++++++++++++++++ 5 files changed, 258 insertions(+) create mode 100644 docs/rules/forbid-component-props.md create mode 100644 lib/rules/forbid-component-props.js create mode 100644 tests/lib/rules/forbid-component-props.js diff --git a/README.md b/README.md index b38d171005..8b6ffbaf62 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](# # List of supported rules * [react/display-name](docs/rules/display-name.md): Prevent missing `displayName` in a React component definition +* [react/forbid-component-props](docs/rules/forbid-component-props.md): Forbid certain props on Components * [react/forbid-prop-types](docs/rules/forbid-prop-types.md): Forbid certain propTypes * [react/no-danger](docs/rules/no-danger.md): Prevent usage of dangerous JSX properties * [react/no-deprecated](docs/rules/no-deprecated.md): Prevent usage of deprecated methods diff --git a/docs/rules/forbid-component-props.md b/docs/rules/forbid-component-props.md new file mode 100644 index 0000000000..7b6990cb13 --- /dev/null +++ b/docs/rules/forbid-component-props.md @@ -0,0 +1,44 @@ +# Forbid certain props on Components (forbid-component-props) + +By default this rule prevents passing of [props that add lots of complexity](https://medium.com/brigade-engineering/don-t-pass-css-classes-between-components-e9f7ab192785) (`className`, `style`) to Components. This rule only applies to Components (e.g. ``) and not DOM nodes (e.g. `
`). The list of forbidden props can be customized with the `forbid` option. + +## Rule Details + +This rule checks all JSX elements and verifies that no forbidden props are used +on Components. This rule is off by default. + +The following patterns are considered warnings: + +```jsx + +``` + +```jsx + +``` + +The following patterns are not considered warnings: + +```jsx + +``` + +```jsx +
+``` + +```jsx +
+``` + +## Rule Options + +```js +... +"forbid-component-props": [, { "forbid": [] }] +... +``` + +### `forbid` + +An array of strings, with the names of props that are forbidden. diff --git a/index.js b/index.js index 76aba279de..931782ceb2 100644 --- a/index.js +++ b/index.js @@ -40,6 +40,7 @@ var rules = { 'jsx-closing-bracket-location': require('./lib/rules/jsx-closing-bracket-location'), 'jsx-space-before-closing': require('./lib/rules/jsx-space-before-closing'), 'no-direct-mutation-state': require('./lib/rules/no-direct-mutation-state'), + 'forbid-component-props': require('./lib/rules/forbid-component-props'), 'forbid-prop-types': require('./lib/rules/forbid-prop-types'), 'prefer-es6-class': require('./lib/rules/prefer-es6-class'), 'jsx-key': require('./lib/rules/jsx-key'), diff --git a/lib/rules/forbid-component-props.js b/lib/rules/forbid-component-props.js new file mode 100644 index 0000000000..c06e9ceff1 --- /dev/null +++ b/lib/rules/forbid-component-props.js @@ -0,0 +1,59 @@ +/** + * @fileoverview Forbid certain props on components + * @author Joe Lencioni + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Constants +// ------------------------------------------------------------------------------ + +var DEFAULTS = ['className', 'style']; + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = function(context) { + + function isForbidden(prop) { + var configuration = context.options[0] || {}; + + var forbid = configuration.forbid || DEFAULTS; + return forbid.indexOf(prop) >= 0; + } + + return { + JSXAttribute: function(node) { + var tag = node.parent.name.name; + if (tag[0] !== tag[0].toUpperCase()) { + // This is a DOM node, not a Component, so exit. + return; + } + + var prop = node.name.name; + + if (!isForbidden(prop)) { + return; + } + + context.report({ + node: node, + message: 'Prop `' + prop + '` is forbidden on Components' + }); + } + }; +}; + +module.exports.schema = [{ + type: 'object', + properties: { + forbid: { + type: 'array', + items: { + type: 'string' + } + } + }, + additionalProperties: true +}]; diff --git a/tests/lib/rules/forbid-component-props.js b/tests/lib/rules/forbid-component-props.js new file mode 100644 index 0000000000..9483f80e4e --- /dev/null +++ b/tests/lib/rules/forbid-component-props.js @@ -0,0 +1,153 @@ +/** + * @fileoverview Tests for forbid-component-props + */ +'use strict'; + +// ----------------------------------------------------------------------------- +// Requirements +// ----------------------------------------------------------------------------- + +var rule = require('../../../lib/rules/forbid-component-props'); +var RuleTester = require('eslint').RuleTester; + +var parserOptions = { + ecmaVersion: 6, + ecmaFeatures: { + experimentalObjectRestSpread: true, + jsx: true + } +}; + +require('babel-eslint'); + +// ----------------------------------------------------------------------------- +// Tests +// ----------------------------------------------------------------------------- + +var CLASSNAME_ERROR_MESSAGE = 'Prop `className` is forbidden on Components'; +var STYLE_ERROR_MESSAGE = 'Prop `style` is forbidden on Components'; + +var ruleTester = new RuleTester(); +ruleTester.run('forbid-component-props', rule, { + + valid: [{ + code: [ + 'var First = React.createClass({', + ' render: function() {', + ' return
;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'var First = React.createClass({', + ' render: function() {', + ' return
;', + ' }', + '});' + ].join('\n'), + options: [{forbid: ['style']}], + parserOptions: parserOptions + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + options: [{forbid: ['style']}], + parserOptions: parserOptions + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + options: [{forbid: ['style', 'foo']}], + parserOptions: parserOptions + }], + + invalid: [{ + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: CLASSNAME_ERROR_MESSAGE, + line: 4, + column: 17, + type: 'JSXAttribute' + }] + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: STYLE_ERROR_MESSAGE, + line: 4, + column: 17, + type: 'JSXAttribute' + }] + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + options: [{forbid: ['className', 'style']}], + errors: [{ + message: CLASSNAME_ERROR_MESSAGE, + line: 4, + column: 17, + type: 'JSXAttribute' + }] + }, { + code: [ + 'var First = React.createClass({', + ' propTypes: externalPropTypes,', + ' render: function() {', + ' return ;', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + options: [{forbid: ['className', 'style']}], + errors: [{ + message: STYLE_ERROR_MESSAGE, + line: 4, + column: 17, + type: 'JSXAttribute' + }] + }] +}); From 7e67c2dfc83bdb8a13b0dec560c1ed46ac39c863 Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Thu, 28 Jul 2016 10:31:08 -0700 Subject: [PATCH 2/3] Update forbid-component-props to new ESLint rule format 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]: https://github.com/eslint/eslint-transforms/issues/2#issuecomment-235948019 This is a completely automated change. --- lib/rules/forbid-component-props.js | 76 ++++++++++++++++------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/lib/rules/forbid-component-props.js b/lib/rules/forbid-component-props.js index c06e9ceff1..abbf527579 100644 --- a/lib/rules/forbid-component-props.js +++ b/lib/rules/forbid-component-props.js @@ -14,46 +14,52 @@ var DEFAULTS = ['className', 'style']; // Rule Definition // ------------------------------------------------------------------------------ -module.exports = function(context) { +module.exports = { + meta: { + docs: {}, - function isForbidden(prop) { - var configuration = context.options[0] || {}; + schema: [{ + type: 'object', + properties: { + forbid: { + type: 'array', + items: { + type: 'string' + } + } + }, + additionalProperties: true + }] + }, - var forbid = configuration.forbid || DEFAULTS; - return forbid.indexOf(prop) >= 0; - } + create: function(context) { - return { - JSXAttribute: function(node) { - var tag = node.parent.name.name; - if (tag[0] !== tag[0].toUpperCase()) { - // This is a DOM node, not a Component, so exit. - return; - } + function isForbidden(prop) { + var configuration = context.options[0] || {}; - var prop = node.name.name; + var forbid = configuration.forbid || DEFAULTS; + return forbid.indexOf(prop) >= 0; + } - if (!isForbidden(prop)) { - return; - } + return { + JSXAttribute: function(node) { + var tag = node.parent.name.name; + if (tag[0] !== tag[0].toUpperCase()) { + // This is a DOM node, not a Component, so exit. + return; + } - context.report({ - node: node, - message: 'Prop `' + prop + '` is forbidden on Components' - }); - } - }; -}; + var prop = node.name.name; + + if (!isForbidden(prop)) { + return; + } -module.exports.schema = [{ - type: 'object', - properties: { - forbid: { - type: 'array', - items: { - type: 'string' + context.report({ + node: node, + message: 'Prop `' + prop + '` is forbidden on Components' + }); } - } - }, - additionalProperties: true -}]; + }; + } +}; From 67919e43ee69e712770fe43c30ea52f84ae71e42 Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Thu, 28 Jul 2016 10:44:09 -0700 Subject: [PATCH 3/3] Mention defaults for `forbid` option in docs Listing the defaults in the documentation for these rules makes it clearer how they work. --- docs/rules/forbid-component-props.md | 2 +- docs/rules/forbid-prop-types.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/forbid-component-props.md b/docs/rules/forbid-component-props.md index 7b6990cb13..924cbe6aaf 100644 --- a/docs/rules/forbid-component-props.md +++ b/docs/rules/forbid-component-props.md @@ -41,4 +41,4 @@ The following patterns are not considered warnings: ### `forbid` -An array of strings, with the names of props that are forbidden. +An array of strings, with the names of props that are forbidden. The default value of this option is `['className', 'style']`. diff --git a/docs/rules/forbid-prop-types.md b/docs/rules/forbid-prop-types.md index 28050726d5..d679e09a88 100644 --- a/docs/rules/forbid-prop-types.md +++ b/docs/rules/forbid-prop-types.md @@ -50,7 +50,7 @@ class Component extends React.Component { ### `forbid` -An array of strings, with the names of `React.PropTypes` keys that are forbidden. +An array of strings, with the names of `React.PropTypes` keys that are forbidden. The default value for this option is `['any', 'array', 'object']`. ## When not to use