diff --git a/README.md b/README.md index b21870ace5..6260580036 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,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/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/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 03f6744b9d..a0ac8bf0b1 100644 --- a/index.js +++ b/index.js @@ -55,6 +55,7 @@ var rules = { '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'), + 'style-prop-object': require('./lib/rules/style-prop-object'), 'no-unused-prop-types': require('./lib/rules/no-unused-prop-types') }; 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) { 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/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/lib/util/Components.js b/lib/util/Components.js index 05c642ba01..c9c6e10aad 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 * @@ -483,14 +496,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 +517,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' } ], 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: [ 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 + } + ] +});