From 7ed3b29427480c2f19e62b3081a3d8b61ad29de0 Mon Sep 17 00:00:00 2001 From: David Petersen Date: Mon, 22 Aug 2016 14:34:25 -0500 Subject: [PATCH 1/4] Add style-prop-object rule (fixes #715) --- README.md | 1 + docs/rules/style-prop-object.md | 51 ++++++++ index.js | 3 +- lib/rules/style-prop-object.js | 80 ++++++++++++ tests/lib/rules/style-prop-object.js | 184 +++++++++++++++++++++++++++ 5 files changed, 318 insertions(+), 1 deletion(-) create mode 100644 docs/rules/style-prop-object.md create mode 100644 lib/rules/style-prop-object.js create mode 100644 tests/lib/rules/style-prop-object.js diff --git a/README.md b/README.md index 3a9cdf9e6b..0ea49a94eb 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](# * [react/self-closing-comp](docs/rules/self-closing-comp.md): Prevent extra closing tags for components without children (fixable) * [react/sort-comp](docs/rules/sort-comp.md): Enforce component methods order * [react/sort-prop-types](docs/rules/sort-prop-types.md): Enforce propTypes declarations alphabetical sorting +* [react/style-prop-object](docs/rules/style-prop-object.md): Enforce style prop value being an object ## JSX-specific rules diff --git a/docs/rules/style-prop-object.md b/docs/rules/style-prop-object.md new file mode 100644 index 0000000000..648c30567d --- /dev/null +++ b/docs/rules/style-prop-object.md @@ -0,0 +1,51 @@ +# Enforce style prop value being an object (style-prop-object) + +Require that the value of the prop `style` be an object or a variable that is +an object. + +## Rule Details + +The following patterns are considered warnings: + +```jsx +
+ +
+ + + +const styles = true; +
+``` + +```js +React.createElement("div", { style: "color: 'red'" }); + +React.createElement("div", { style: true }); + +React.createElement("Hello", { style: true }); + +const styles = true; +React.createElement("div", { style: styles }); +``` + + +The following patterns are not considered warnings: + +```jsx +
+ + + +const styles = { color: "red" }; +
+``` + +```js +React.createElement("div", { style: { color: 'red' }}); + +React.createElement("Hello", { style: { color: 'red' }}); + +const styles = { height: '100px' }; +React.createElement("div", { style: styles }); +``` diff --git a/index.js b/index.js index 88ab4dcb6c..8d221d2c8b 100644 --- a/index.js +++ b/index.js @@ -54,7 +54,8 @@ var rules = { 'jsx-filename-extension': require('./lib/rules/jsx-filename-extension'), 'require-optimization': require('./lib/rules/require-optimization'), 'no-find-dom-node': require('./lib/rules/no-find-dom-node'), - 'no-danger-with-children': require('./lib/rules/no-danger-with-children') + 'no-danger-with-children': require('./lib/rules/no-danger-with-children'), + 'style-prop-object': require('./lib/rules/style-prop-object') }; var ruleNames = Object.keys(rules); diff --git a/lib/rules/style-prop-object.js b/lib/rules/style-prop-object.js new file mode 100644 index 0000000000..11c9fb1e15 --- /dev/null +++ b/lib/rules/style-prop-object.js @@ -0,0 +1,80 @@ +/** + * @fileoverview Enforce style prop value is an object + * @author David Petersen + */ +'use strict'; + +var variableUtil = require('../util/variable'); + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'Enforce style prop value is an object', + category: '', + recommended: false + }, + schema: [] + }, + + create: function(context) { + /** + * @param {object} node A Identifier node + */ + function checkIdentifiers(node) { + var variable = variableUtil.variablesInScope(context).find(function (item) { + return item.name === node.name; + }); + + if (!variable || !variable.defs[0].node.init) { + return; + } + + if (variable.defs[0].node.init.type === 'Literal') { + context.report(node, 'Style prop value must be an object'); + } + } + + return { + CallExpression: function(node) { + if ( + node.callee + && node.callee.type === 'MemberExpression' + && node.callee.property.name === 'createElement' + && node.arguments.length > 1 + ) { + if (node.arguments[1].type === 'ObjectExpression') { + var style = node.arguments[1].properties.find(function(property) { + return property.key.name === 'style'; + }); + if (style) { + if (style.value.type === 'Identifier') { + checkIdentifiers(style.value); + } else if (style.value.type === 'Literal') { + context.report(style.value, 'Style prop value must be an object'); + } + } + } + } + }, + + JSXAttribute: function(node) { + if (node.name.name !== 'style') { + return; + } + + if ( + node.value.type !== 'JSXExpressionContainer' + || node.value.expression.type === 'Literal' + ) { + context.report(node, 'Style prop value must be an object'); + } else if (node.value.expression.type === 'Identifier') { + checkIdentifiers(node.value.expression); + } + } + }; + } +}; diff --git a/tests/lib/rules/style-prop-object.js b/tests/lib/rules/style-prop-object.js new file mode 100644 index 0000000000..eda9966775 --- /dev/null +++ b/tests/lib/rules/style-prop-object.js @@ -0,0 +1,184 @@ +/** + * @fileoverview Enforce style prop value is an object + * @author David Petersen + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +var rule = require('../../../lib/rules/style-prop-object'); +var RuleTester = require('eslint').RuleTester; + +var parserOptions = { + ecmaVersion: 6, + ecmaFeatures: { + experimentalObjectRestSpread: true, + jsx: true + } +}; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +var ruleTester = new RuleTester(); +ruleTester.run('style-prop-object', rule, { + valid: [ + { + code: '
', + parserOptions: parserOptions + }, + { + code: '', + parserOptions: parserOptions + }, + { + code: [ + 'function redDiv() {', + ' const styles = { color: "red" };', + ' return
;', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'function redDiv() {', + ' const styles = { color: "red" };', + ' return ;', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'const styles = { color: "red" };', + 'function redDiv() {', + ' return
;', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'function redDiv(props) {', + ' return
;', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'import styles from \'./styles\';', + 'function redDiv() {', + ' return
;', + '}' + ].join('\n'), + parserOptions: Object.assign({sourceType: 'module'}, parserOptions) + }, + { + code: [ + 'import mystyles from \'./styles\';', + 'const styles = Object.assign({ color: \'red\' }, mystyles);', + 'function redDiv() {', + ' return
;', + '}' + ].join('\n'), + parserOptions: Object.assign({sourceType: 'module'}, parserOptions) + }, + { + code: [ + 'const otherProps = { style: { color: "red" } };', + 'const { a, b, ...props } = otherProps;', + '
' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'const styles = Object.assign({ color: \'red\' }, mystyles);', + 'React.createElement("div", { style: styles });' + ].join('\n'), + parserOptions: Object.assign({sourceType: 'module'}, parserOptions) + } + ], + invalid: [ + { + code: '
', + errors: [{ + message: 'Style prop value must be an object', + line: 1, + column: 6, + type: 'JSXAttribute' + }], + parserOptions: parserOptions + }, + { + code: '', + errors: [{ + message: 'Style prop value must be an object', + line: 1, + column: 8, + type: 'JSXAttribute' + }], + parserOptions: parserOptions + }, + { + code: '
', + errors: [{ + message: 'Style prop value must be an object', + line: 1, + column: 6, + type: 'JSXAttribute' + }], + parserOptions: parserOptions + }, + { + code: [ + 'const styles = \'color: "red"\';', + 'function redDiv2() {', + ' return
;', + '}' + ].join('\n'), + errors: [{ + message: 'Style prop value must be an object', + line: 3, + column: 22, + type: 'Identifier' + }], + parserOptions: parserOptions + }, + { + code: [ + 'const styles = \'color: "red"\';', + 'function redDiv2() {', + ' return ;', + '}' + ].join('\n'), + errors: [{ + message: 'Style prop value must be an object', + line: 3, + column: 24, + type: 'Identifier' + }], + parserOptions: parserOptions + }, + { + code: [ + 'const styles = true;', + 'function redDiv() {', + ' return
;', + '}' + ].join('\n'), + errors: [{ + message: 'Style prop value must be an object', + line: 3, + column: 22, + type: 'Identifier' + }], + parserOptions: parserOptions + } + ] +}); From 70bf28d89eae53b53e3bf41c4bae84cb8700f984 Mon Sep 17 00:00:00 2001 From: Yannick Croissant Date: Mon, 22 Aug 2016 21:00:59 +0000 Subject: [PATCH 2/4] Fix component detection to ignore functions expression without a parent component --- lib/util/Components.js | 24 ++++++++++++++---------- tests/lib/rules/display-name.js | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/lib/util/Components.js b/lib/util/Components.js index 80f4a1e871..da64d4e3cc 100644 --- a/lib/util/Components.js +++ b/lib/util/Components.js @@ -483,14 +483,16 @@ function componentRule(rule, context) { }, FunctionExpression: function(node) { - node = utils.getParentComponent(); + var component = utils.getParentComponent(); if ( - !node || - (node.parent && node.parent.type === 'JSXExpressionContainer') + !component || + (component.parent && component.parent.type === 'JSXExpressionContainer') ) { + // Ban the node if we cannot find a parent component + components.add(node, 0); return; } - components.add(node, 1); + components.add(component, 1); }, FunctionDeclaration: function(node) { @@ -502,17 +504,19 @@ function componentRule(rule, context) { }, ArrowFunctionExpression: function(node) { - node = utils.getParentComponent(); + var component = utils.getParentComponent(); if ( - !node || - (node.parent && node.parent.type === 'JSXExpressionContainer') + !component || + (component.parent && component.parent.type === 'JSXExpressionContainer') ) { + // Ban the node if we cannot find a parent component + components.add(node, 0); return; } - if (node.expression && utils.isReturningJSX(node)) { - components.add(node, 2); + if (component.expression && utils.isReturningJSX(component)) { + components.add(component, 2); } else { - components.add(node, 1); + components.add(component, 1); } }, diff --git a/tests/lib/rules/display-name.js b/tests/lib/rules/display-name.js index 1d83108808..083cee786f 100644 --- a/tests/lib/rules/display-name.js +++ b/tests/lib/rules/display-name.js @@ -358,6 +358,26 @@ ruleTester.run('display-name', rule, { 'module.exports = someDecorator;' ].join('\n'), parser: 'babel-eslint' + }, { + code: [ + 'const element = (', + ' {', + ' renderWasCalled = true', + ' return
', + ' }}/>', + ')' + ].join('\n'), + parser: 'babel-eslint' + }, { + code: [ + 'const element = (', + ' ', + ' }}/>', + ')' + ].join('\n'), + parser: 'babel-eslint' } ], From d4dee487b92aebe7377a38cb6493c44ebc3569b8 Mon Sep 17 00:00:00 2001 From: Yannick Croissant Date: Tue, 23 Aug 2016 19:40:20 +0000 Subject: [PATCH 3/4] Fix jsx-uses-vars to work better with prefer-const (fixes #716) --- lib/rules/jsx-uses-vars.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/rules/jsx-uses-vars.js b/lib/rules/jsx-uses-vars.js index 530e1b48cd..2433410e20 100644 --- a/lib/rules/jsx-uses-vars.js +++ b/lib/rules/jsx-uses-vars.js @@ -23,13 +23,6 @@ module.exports = { create: function(context) { return { - JSXExpressionContainer: function(node) { - if (node.expression.type !== 'Identifier') { - return; - } - variableUtil.markVariableAsUsed(context, node.expression.name); - }, - JSXOpeningElement: function(node) { var name; if (node.name.namespace && node.name.namespace.name) { From 7024678853b5e2650ccf9c1df33d20e2120b6450 Mon Sep 17 00:00:00 2001 From: Tieme van Veen Date: Tue, 23 Aug 2016 23:00:44 +0200 Subject: [PATCH 4/4] Add PureComponent support to prefer-stateless-function --- docs/rules/prefer-stateless-function.md | 51 +++++++++++++++--- lib/rules/no-multi-comp.js | 2 +- lib/rules/prefer-stateless-function.js | 51 ++++++++++++++++-- lib/rules/require-optimization.js | 22 +------- lib/util/Components.js | 13 +++++ tests/lib/rules/prefer-stateless-function.js | 55 ++++++++++++++++++++ 6 files changed, 163 insertions(+), 31 deletions(-) diff --git a/docs/rules/prefer-stateless-function.md b/docs/rules/prefer-stateless-function.md index 5bedda460c..b42c3c0ca7 100644 --- a/docs/rules/prefer-stateless-function.md +++ b/docs/rules/prefer-stateless-function.md @@ -8,14 +8,15 @@ This rule will check your class based React components for * methods/properties other than `displayName`, `propTypes`, `render` and useless constructor (same detection as ESLint [no-useless-constructor rule](http://eslint.org/docs/rules/no-useless-constructor)) * instance property other than `this.props` and `this.context` +* extension of `React.PureComponent` () * presence of `ref` attribute in JSX * `render` method that return anything but JSX: `undefined`, `null`, etc. (only in React <15.0.0, see [shared settings](https://github.com/yannickcr/eslint-plugin-react/blob/master/README.md#configuration) for React version configuration) -If none of these 4 elements are found, the rule will warn you to write this component as a pure function. +If none of these elements are found, the rule will warn you to write this component as a pure function. -The following pattern is considered warnings: +The following pattern is considered a warning: -```js +```jsx var Hello = React.createClass({ render: function() { return
Hello {this.props.name}
; @@ -23,17 +24,17 @@ var Hello = React.createClass({ }); ``` -The following pattern is not considered warnings: +The following pattern is not considered a warning: -```js +```jsx const Foo = function(props) { return
{props.foo}
; }; ``` -The following pattern is not considered warning in React <15.0.0: +The following pattern is not considered a warning in React <15.0.0: -```js +```jsx class Foo extends React.Component { render() { if (!this.props.foo) { @@ -43,3 +44,39 @@ class Foo extends React.Component { } } ``` + + +## Rule Options + +```js +... +"prefer-stateless-function": [, { "ignorePureComponent": }] +... +``` + +* `enabled`: for enabling the rule. 0=off, 1=warn, 2=error. Defaults to 0. +* `ignorePureComponent`: optional boolean set to `true` to ignore components extending from `React.PureComponent` (default to `false`). + +### `ignorePureComponent` + +When `true` the rule will ignore Components extending from `React.PureComponent` that use `this.props` or `this.context`. + +The following patterns is considered okay and does not cause warnings: + +```jsx +class Foo extends React.PureComponent { + render() { + return
{this.props.foo}
; + } +} +``` + +The following pattern is considered a warning because it's not using props or context: + +```jsx +class Foo extends React.PureComponent { + render() { + return
Bar
; + } +} +``` diff --git a/lib/rules/no-multi-comp.js b/lib/rules/no-multi-comp.js index 25c160f1ec..2f165fe32c 100644 --- a/lib/rules/no-multi-comp.js +++ b/lib/rules/no-multi-comp.js @@ -43,7 +43,7 @@ module.exports = { * @returns {Boolean} True if the component is ignored, false if not. */ function isIgnored(component) { - return ignoreStateless === true && /Function/.test(component.node.type); + return ignoreStateless && /Function/.test(component.node.type); } // -------------------------------------------------------------------------- diff --git a/lib/rules/prefer-stateless-function.js b/lib/rules/prefer-stateless-function.js index c05b9b0716..e4f2ecc9a4 100644 --- a/lib/rules/prefer-stateless-function.js +++ b/lib/rules/prefer-stateless-function.js @@ -20,11 +20,23 @@ module.exports = { category: 'Stylistic Issues', recommended: false }, - schema: [] + schema: [{ + type: 'object', + properties: { + ignorePureComponents: { + default: false, + type: 'boolean' + } + }, + additionalProperties: false + }] }, create: Components.detect(function(context, components, utils) { + var configuration = context.options[0] || {}; + var ignorePureComponents = configuration.ignorePureComponents || false; + var sourceCode = context.getSourceCode(); // -------------------------------------------------------------------------- @@ -213,6 +225,16 @@ module.exports = { }); } + /** + * Mark component as pure as declared + * @param {ASTNode} node The AST node being checked. + */ + var markSCUAsDeclared = function (node) { + components.set(node, { + hasSCU: true + }); + }; + /** * Mark a setState as used * @param {ASTNode} node The AST node being checked. @@ -223,6 +245,16 @@ module.exports = { }); } + /** + * Mark a props or context as used + * @param {ASTNode} node The AST node being checked. + */ + function markPropsOrContextAsUsed(node) { + components.set(node, { + usePropsOrContext: true + }); + } + /** * Mark a ref as used * @param {ASTNode} node The AST node being checked. @@ -244,6 +276,12 @@ module.exports = { } return { + ClassDeclaration: function (node) { + if (ignorePureComponents && utils.isPureComponent(node)) { + markSCUAsDeclared(node); + } + }, + // Mark `this` destructuring as a usage of `this` VariableDeclarator: function(node) { // Ignore destructuring on other than `this` @@ -256,6 +294,7 @@ module.exports = { return name !== 'props' && name !== 'context'; }); if (!useThis) { + markPropsOrContextAsUsed(node); return; } markThisAsUsed(node); @@ -264,11 +303,13 @@ module.exports = { // Mark `this` usage MemberExpression: function(node) { // Ignore calls to `this.props` and `this.context` - if ( - node.object.type !== 'ThisExpression' || + if (node.object.type !== 'ThisExpression') { + return; + } else if ( (node.property.name || node.property.value) === 'props' || (node.property.name || node.property.value) === 'context' ) { + markPropsOrContextAsUsed(node); return; } markThisAsUsed(node); @@ -322,6 +363,10 @@ module.exports = { continue; } + if (list[component].hasSCU && list[component].usePropsOrContext) { + continue; + } + context.report({ node: list[component].node, message: 'Component should be written as a pure function' diff --git a/lib/rules/require-optimization.js b/lib/rules/require-optimization.js index afb80ca061..e843e278ff 100644 --- a/lib/rules/require-optimization.js +++ b/lib/rules/require-optimization.js @@ -5,7 +5,6 @@ 'use strict'; var Components = require('../util/Components'); -var pragmaUtil = require('../util/pragma'); module.exports = { meta: { @@ -29,15 +28,11 @@ module.exports = { }] }, - create: Components.detect(function (context, components) { + create: Components.detect(function (context, components, utils) { var MISSING_MESSAGE = 'Component is not optimized. Please add a shouldComponentUpdate method.'; var configuration = context.options[0] || {}; var allowDecorators = configuration.allowDecorators || []; - var pragma = pragmaUtil.getFromContext(context); - var pureComponentRegExp = new RegExp('^(' + pragma + '\\.)?PureComponent$'); - var sourceCode = context.getSourceCode(); - /** * Checks to see if our component is decorated by PureRenderMixin via reactMixin * @param {ASTNode} node The AST node being checked. @@ -89,19 +84,6 @@ module.exports = { return false; }; - /** - * Checks to see if our component extends React.PureComponent - * @param {ASTNode} node The AST node being checked. - * @returns {Boolean} True if node extends React.PureComponent, false if not. - */ - var isPureComponent = function (node) { - if (node.superClass) { - return pureComponentRegExp.test(sourceCode.getText(node.superClass)); - } - - return false; - }; - /** * Checks if we are declaring a shouldComponentUpdate method * @param {ASTNode} node The AST node being checked. @@ -186,7 +168,7 @@ module.exports = { }, ClassDeclaration: function (node) { - if (!(hasPureRenderDecorator(node) || hasCustomDecorator(node) || isPureComponent(node))) { + if (!(hasPureRenderDecorator(node) || hasCustomDecorator(node) || utils.isPureComponent(node))) { return; } markSCUAsDeclared(node); diff --git a/lib/util/Components.js b/lib/util/Components.js index da64d4e3cc..129c54a072 100644 --- a/lib/util/Components.js +++ b/lib/util/Components.js @@ -199,6 +199,19 @@ function componentRule(rule, context) { return relevantTags.length > 0; }, + /** + * Checks to see if our component extends React.PureComponent + * + * @param {ASTNode} node The AST node being checked. + * @returns {Boolean} True if node extends React.PureComponent, false if not + */ + isPureComponent: function (node) { + if (node.superClass) { + return new RegExp('^(' + pragma + '\\.)?PureComponent$').test(sourceCode.getText(node.superClass)); + } + return false; + }, + /** * Check if the node is returning JSX * diff --git a/tests/lib/rules/prefer-stateless-function.js b/tests/lib/rules/prefer-stateless-function.js index 38981efcfc..d2f945b50f 100644 --- a/tests/lib/rules/prefer-stateless-function.js +++ b/tests/lib/rules/prefer-stateless-function.js @@ -38,6 +38,32 @@ ruleTester.run('prefer-stateless-function', rule, { // Already a stateless (arrow) function code: 'const Foo = ({foo}) =>
{foo}
;', parserOptions: parserOptions + }, { + // Extends from PureComponent and uses props + code: [ + 'class Foo extends React.PureComponent {', + ' render() {', + ' return
{this.props.foo}
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + options: [{ + ignorePureComponents: true + }] + }, { + // Extends from PureComponent and uses context + code: [ + 'class Foo extends React.PureComponent {', + ' render() {', + ' return
{this.context.foo}
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + options: [{ + ignorePureComponents: true + }] }, { // Has a lifecyle method code: [ @@ -259,6 +285,35 @@ ruleTester.run('prefer-stateless-function', rule, { errors: [{ message: 'Component should be written as a pure function' }] + }, { + // Only extend PureComponent without use of props or context + code: [ + 'class Foo extends React.PureComponent {', + ' render() {', + ' return
foo
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + options: [{ + ignorePureComponents: true + }], + errors: [{ + message: 'Component should be written as a pure function' + }] + }, { + // Extends from PureComponent but no ignorePureComponents option + code: [ + 'class Foo extends React.PureComponent {', + ' render() {', + ' return
{this.props.foo}
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Component should be written as a pure function' + }] }, { // Has only displayName (method) and render code: [