diff --git a/README.md b/README.md index d9711d5805..e2577ed5ee 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,7 @@ Enable the rules that you would like to use. * [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 (fixable) * [react/sort-prop-types](docs/rules/sort-prop-types.md): Enforce propTypes declarations alphabetical sorting +* [react/state-in-constructor](docs/rules/state-in-constructor.md): Enforce the state initialization style to be either in a constructor or with a class property * [react/style-prop-object](docs/rules/style-prop-object.md): Enforce style prop value being an object * [react/void-dom-elements-no-children](docs/rules/void-dom-elements-no-children.md): Prevent void DOM elements (e.g. ``, `
`) from receiving children diff --git a/docs/rules/state-in-constructor.md b/docs/rules/state-in-constructor.md new file mode 100644 index 0000000000..395862402d --- /dev/null +++ b/docs/rules/state-in-constructor.md @@ -0,0 +1,74 @@ +# Enforce state initialization style (react/state-in-constructor) + +This rule will enforce the state initialization style to be either in a constructor or with a class property. + +## Rule Options + +```js +... +"react/state-in-constructor": [, ] +... +``` + +### `always` mode + +Will enforce the state initialization style to be in a constructor. This is the default mode. + +The following patterns are considered warnings: + +```jsx +class Foo extends React.Component { + state = { bar: 0 } + render() { + return
Foo
+ } +} +``` + +The following patterns are **not** considered warnings: + +```jsx +class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + render() { + return
Foo
+ } +} +``` + +### `never` mode + +Will enforce the state initialization style to be with a class property. + +The following patterns are considered warnings: + +```jsx +class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + render() { + return
Foo
+ } +} +``` + +The following patterns are **not** considered warnings: + +```jsx +class Foo extends React.Component { + state = { bar: 0 } + render() { + return
Foo
+ } +} +``` + + +## When Not To Use It + +When the way a component state is being initialized doesn't matter. diff --git a/index.js b/index.js index b4aba79ee1..9c6746896b 100644 --- a/index.js +++ b/index.js @@ -80,6 +80,7 @@ const allRules = { 'self-closing-comp': require('./lib/rules/self-closing-comp'), 'sort-comp': require('./lib/rules/sort-comp'), 'sort-prop-types': require('./lib/rules/sort-prop-types'), + 'state-in-constructor': require('./lib/rules/state-in-constructor'), 'style-prop-object': require('./lib/rules/style-prop-object'), 'void-dom-elements-no-children': require('./lib/rules/void-dom-elements-no-children') }; diff --git a/lib/rules/no-direct-mutation-state.js b/lib/rules/no-direct-mutation-state.js index a4daed44ec..bbb9c62a12 100644 --- a/lib/rules/no-direct-mutation-state.js +++ b/lib/rules/no-direct-mutation-state.js @@ -59,15 +59,6 @@ module.exports = { return node; } - /** - * Determine if this MemberExpression is for `this.state` - * @param {Object} node The node to process - * @returns {Boolean} - */ - function isStateMemberExpression(node) { - return node.object.type === 'ThisExpression' && node.property.name === 'state'; - } - /** * Determine if we should currently ignore assignments in this component. * @param {?Object} component The component to process @@ -101,7 +92,7 @@ module.exports = { return; } const item = getOuterMemberExpression(node.left); - if (isStateMemberExpression(item)) { + if (utils.isStateMemberExpression(item)) { const mutations = (component && component.mutations) || []; mutations.push(node.left.object); components.set(node, { @@ -117,7 +108,7 @@ module.exports = { return; } const item = getOuterMemberExpression(node.argument); - if (isStateMemberExpression(item)) { + if (utils.isStateMemberExpression(item)) { const mutations = (component && component.mutations) || []; mutations.push(item); components.set(node, { diff --git a/lib/rules/state-in-constructor.js b/lib/rules/state-in-constructor.js new file mode 100644 index 0000000000..ebfeb65605 --- /dev/null +++ b/lib/rules/state-in-constructor.js @@ -0,0 +1,58 @@ +/** + * @fileoverview Enforce the state initialization style to be either in a constructor or with a class property + * @author Kanitkorn Sujautra + */ +'use strict'; + +const Components = require('../util/Components'); +const docsUrl = require('../util/docsUrl'); + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'State initialization in an ES6 class component should be in a constructor', + category: 'Stylistic Issues', + recommended: false, + url: docsUrl('state-in-constructor') + }, + schema: [{ + enum: ['always', 'never'] + }] + }, + + create: Components.detect((context, components, utils) => { + const option = context.options[0] || 'always'; + return { + ClassProperty: function (node) { + if ( + option === 'always' && + !node.static && + node.key.name === 'state' && + utils.getParentES6Component() + ) { + context.report({ + node, + message: 'State initialization should be in a constructor' + }); + } + }, + AssignmentExpression(node) { + if ( + option === 'never' && + utils.isStateMemberExpression(node.left) && + utils.inConstructor() && + utils.getParentES6Component() + ) { + context.report({ + node, + message: 'State initialization should be in a class property' + }); + } + } + }; + }) +}; diff --git a/lib/util/Components.js b/lib/util/Components.js index 7e83f3f248..38459a0ffa 100644 --- a/lib/util/Components.js +++ b/lib/util/Components.js @@ -298,6 +298,30 @@ function componentRule(rule, context) { return calledOnPragma; }, + /** + * Check if we are in a class constructor + * @return {boolean} true if we are in a class constructor, false if not + */ + inConstructor: function() { + let scope = context.getScope(); + while (scope) { + if (scope.block && scope.block.parent && scope.block.parent.kind === 'constructor') { + return true; + } + scope = scope.upper; + } + return false; + }, + + /** + * Determine if the node is MemberExpression of `this.state` + * @param {Object} node The node to process + * @returns {Boolean} + */ + isStateMemberExpression: function(node) { + return node.type === 'MemberExpression' && node.object.type === 'ThisExpression' && node.property.name === 'state'; + }, + getReturnPropertyAndNode(ASTnode) { let property; let node = ASTnode; diff --git a/lib/util/usedPropTypes.js b/lib/util/usedPropTypes.js index e8d74d1e21..b24f5238b0 100644 --- a/lib/util/usedPropTypes.js +++ b/lib/util/usedPropTypes.js @@ -200,21 +200,6 @@ module.exports = function usedPropTypesInstructions(context, components, utils) return key.type === 'Identifier' ? key.name : key.value; } - /** - * Check if we are in a class constructor - * @return {boolean} true if we are in a class constructor, false if not - */ - function inConstructor() { - let scope = context.getScope(); - while (scope) { - if (scope.block && scope.block.parent && scope.block.parent.kind === 'constructor') { - return true; - } - scope = scope.upper; - } - return false; - } - /** * Retrieve the name of a property node * @param {ASTNode} node The AST node with the property. @@ -226,7 +211,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) const isDirectPrevProp = DIRECT_PREV_PROPS_REGEX.test(sourceCode.getText(node)); const isDirectSetStateProp = isPropArgumentInSetStateUpdater(node); const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component(); - const isNotInConstructor = !inConstructor(node); + const isNotInConstructor = !utils.inConstructor(node); const isNotInLifeCycleMethod = !inLifeCycleMethod(); const isNotInSetStateUpdater = !inSetStateUpdater(); if ((isDirectProp || isDirectNextProp || isDirectPrevProp || isDirectSetStateProp) @@ -383,7 +368,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) || DIRECT_NEXT_PROPS_REGEX.test(nodeSource) || DIRECT_PREV_PROPS_REGEX.test(nodeSource); const reportedNode = ( - !isDirectProp && !inConstructor() && !inComponentWillReceiveProps() ? + !isDirectProp && !utils.inConstructor() && !inComponentWillReceiveProps() ? node.parent.property : node.property ); diff --git a/tests/lib/rules/state-in-constructor.js b/tests/lib/rules/state-in-constructor.js new file mode 100644 index 0000000000..8324b56cf8 --- /dev/null +++ b/tests/lib/rules/state-in-constructor.js @@ -0,0 +1,354 @@ +/** + * @fileoverview Enforce the state initialization style to be either in a constructor or with a class property + * @author Kanitkorn Sujautra + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/state-in-constructor'); +const RuleTester = require('eslint').RuleTester; + +const ruleTesterConfig = { + parser: 'babel-eslint', + parserOptions: { + ecmaVersion: 2018, + sourceType: 'module', + ecmaFeatures: { + jsx: true + } + } +}; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const ruleTester = new RuleTester(ruleTesterConfig); +ruleTester.run('state-in-constructor', rule, { + valid: [{ + code: ` + class Foo extends React.Component { + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + render() { + return
Foo
+ } + } + `, + options: ['always'] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + baz = { bar: 0 } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.baz = { bar: 0 } + } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.baz = { bar: 0 } + } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + baz = { bar: 0 } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + baz = { bar: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + const Foo = () =>
Foo
+ ` + }, { + code: ` + const Foo = () =>
Foo
+ `, + options: ['never'] + }, { + code: ` + function Foo () { + return
Foo
+ } + ` + }, { + code: ` + function Foo () { + return
Foo
+ } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + state = { bar: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + state = { bar: 0 } + baz = { bar: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.baz = { bar: 0 } + } + state = { baz: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + if (foobar) { + this.state = { bar: 0 } + } + } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + foobar = { bar: 0 } + } + render() { + return
Foo
+ } + } + ` + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + foobar = { bar: 0 } + } + render() { + return
Foo
+ } + } + `, + options: ['never'] + }], + + invalid: [{ + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + render() { + return
Foo
+ } + } + `, + options: ['never'], + errors: [{ + message: 'State initialization should be in a class property' + }] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + baz = { bar: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'], + errors: [{ + message: 'State initialization should be in a class property' + }] + }, { + code: ` + class Foo extends React.Component { + state = { bar: 0 } + render() { + return
Foo
+ } + } + `, + errors: [{ + message: 'State initialization should be in a constructor' + }] + }, { + code: ` + class Foo extends React.Component { + state = { bar: 0 } + baz = { bar: 0 } + render() { + return
Foo
+ } + } + `, + errors: [{ + message: 'State initialization should be in a constructor' + }] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.baz = { bar: 0 } + } + state = { baz: 0 } + render() { + return
Foo
+ } + } + `, + errors: [{ + message: 'State initialization should be in a constructor' + }] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + state = { baz: 0 } + render() { + return
Foo
+ } + } + `, + errors: [{ + message: 'State initialization should be in a constructor' + }] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + this.state = { bar: 0 } + } + state = { baz: 0 } + render() { + return
Foo
+ } + } + `, + options: ['never'], + errors: [{ + message: 'State initialization should be in a class property' + }] + }, { + code: ` + class Foo extends React.Component { + constructor(props) { + super(props) + if (foobar) { + this.state = { bar: 0 } + } + } + render() { + return
Foo
+ } + } + `, + options: ['never'], + errors: [{ + message: 'State initialization should be in a class property' + }] + }] +});