diff --git a/docs/rules/no-mutation-props.md b/docs/rules/no-mutation-props.md
index d6b23082a8..e21260ffce 100644
--- a/docs/rules/no-mutation-props.md
+++ b/docs/rules/no-mutation-props.md
@@ -16,4 +16,41 @@ var Hello = React.createClass({
return
Hello {this.props.name}
;
}
});
+
+var Hello = React.createClass({
+ render: function() {
+ const {list} = this.props;
+ list.push(2);
+ return {list.length} things
;
+ }
+});
+
+var Hello = React.createClass({
+ render: function() {
+ const [firstThing] = this.props.list;
+ firstThing.foo = 'bar';
+ return {firstThing.foo}
;
+ }
+});
+
+var Hello = React.createClass({
+ render: function() {
+ delete this.props.foo;
+ return {Object.keys(this.props).length} props
;
+ }
+});
+
+var Hello = React.createClass({
+ render: function() {
+ Object.assign(this.props.foo, {bar: 'baz'})
+ return {this.props.foo.bar}
;
+ }
+});
+
+var Hello = React.createClass({
+ render: function() {
+ Object.defineProperty(this.props, 'foo')
+ return {this.props.foo}
;
+ }
+});
```
diff --git a/lib/rules/no-mutation-props.js b/lib/rules/no-mutation-props.js
index c669478887..c39c74d880 100644
--- a/lib/rules/no-mutation-props.js
+++ b/lib/rules/no-mutation-props.js
@@ -1,10 +1,11 @@
/**
* @fileoverview Prevent mutation of this.props
- * @author Ian Schmitz
+ * @author Ian Schmitz, Joey Baker
*/
'use strict';
-var Components = require('../util/Components');
+const Components = require('../util/Components');
+const ARRAY_MUTATIONS = ['push', 'pop', 'shift', 'unshift'];
// ------------------------------------------------------------------------------
// Rule Definition
@@ -19,18 +20,65 @@ module.exports = {
}
},
- create: Components.detect(function (context, components, utils) {
+ create: Components.detect((context, components, utils) => {
+ /**
+ * Determines if a given node is from `this.props`
+ * @param {Object} node The node to examine
+ */
+ function isPropsNode (node) {
+ if (!node) {
+ return false;
+ }
+
+ let topObject = node;
+
+ // when looking at an object path, get the top-most object
+ while (topObject.object) {
+ if (topObject.object && topObject.object.object) {
+ topObject = topObject.object;
+ } else {
+ break;
+ }
+ }
+
+ // if this is a simple `this.props` call, we can bail early
+ if (topObject.object && topObject.object.type === 'ThisExpression') {
+ return topObject.property && topObject.property.name === 'props';
+ }
+
+ // if this is a variable, we need to look at the scope to determine if
+ // it is actually a reference to a prop
+ const name = topObject.name || (topObject.object && topObject.object.name);
+ const originalDeclaration = context.getScope().variableScope.set.get(name);
+ if (originalDeclaration) {
+ const originalIdentifiers = originalDeclaration.identifiers;
+ // if the get call returns falsey, we've not set it in our cache
+ // and it can't have have been from props. See `VariableDeclaration`
+ const currentIdentifier = components.get(originalIdentifiers[originalIdentifiers.length - 1]);
+ return currentIdentifier && currentIdentifier.isFromProps;
+ }
+
+ return false;
+ }
+
+ function setPropMutation(mutation, node) {
+ const component = components.get(utils.getParentComponent());
+ const propMutations = component && component.propMutations
+ ? component.propMutations.concat([mutation])
+ : [mutation];
+ components.set(node || mutation, {propMutations});
+ }
/**
* Reports this.props mutations for a given component
* @param {Object} component The component to process
*/
function reportMutations(component) {
- if (!component.mutations) {
+ if (!component.propMutations) {
return;
}
- component.mutations.forEach(function(mutation) {
+ component.propMutations.forEach(mutation => {
context.report({
node: mutation,
message: 'A component must never modify its own props.'
@@ -43,38 +91,103 @@ module.exports = {
// --------------------------------------------------------------------------
return {
- AssignmentExpression: function (node) {
- var item;
+ VariableDeclaration: function(node) {
+ if (!node.declarations) {
+ return;
+ }
+
+ node.declarations.forEach(declaration => {
+ const init = declaration.init;
+ const id = declaration.id;
+ const isProps = isPropsNode(init);
+ if (!isProps) {
+ return;
+ }
+ const assignments =
+ // if this is a simple assignment `const b = this.props.b`
+ (id.type === 'Identifier' && [id])
+ // if this is object destructuring `const {b} = this.props`
+ || (id.type === 'ObjectPattern' && id.properties.map(property => property.value))
+ // if this is array destructuring `const [b] = this.props.list`
+ || (id.type === 'ArrayPattern' && id.elements);
+
+ // remember that this node is a prop, `isFromProps` relies on this
+ assignments.forEach(assignment => {
+ components.add(assignment);
+ components.set(assignment, {
+ isFromProps: true
+ });
+ });
+ });
+ },
- if (!node.left || !node.left.object || !node.left.object.object) {
+ UnaryExpression: function(node) {
+ const isDeleteOperation = node.operator === 'delete';
+ if (!isDeleteOperation) {
+ return;
+ }
+ const isProp = isPropsNode(node.argument);
+ if (!isProp) {
return;
}
- item = node.left.object;
- while (item.object && item.object.property) {
- item = item.object;
+ setPropMutation(node);
+ },
+
+ CallExpression: function(node) {
+ const callee = node.callee;
+ const args = node.arguments;
+ const calleeName = callee.property && callee.property.name;
+
+ const isObjectMutation =
+ callee.type === 'MemberExpression'
+ && callee.object.name === 'Object'
+ && (
+ calleeName === 'assign'
+ || calleeName === 'defineProperty'
+ );
+
+ const isArrayMutation =
+ callee.type === 'MemberExpression'
+ && ARRAY_MUTATIONS.indexOf(calleeName) > -1;
+
+ if (!isObjectMutation && !isArrayMutation) {
+ return;
}
- if (item.object.type === 'ThisExpression' && item.property.name === 'props') {
- var component = components.get(utils.getParentComponent());
- var mutations = component && component.mutations || [];
- mutations.push(node.left.object);
- components.set(node, {
- mutations: mutations
- });
+ const isCloneOperation = args.length && args[0].type === 'ObjectExpression';
+
+ if (isObjectMutation && isCloneOperation) {
+ return;
+ }
+
+ // object mutations are a function call where the possible prop is
+ // always the first arg. Array mutations are always method calls on the
+ // possible prop
+ const isProps = isPropsNode(isObjectMutation ? args[0] : callee.object);
+
+ if (!isProps) {
+ return;
+ }
+
+ setPropMutation(node);
+ },
+
+ AssignmentExpression: function (node) {
+ if (isPropsNode(node.left)) {
+ setPropMutation(node.left.object, node);
}
},
'Program:exit': function () {
- var list = components.list();
+ const list = components.list();
- Object.keys(list).forEach(function (key) {
- var component = list[key];
+ Object.keys(list).forEach(key => {
+ const component = list[key];
reportMutations(component);
});
}
};
-
})
};
diff --git a/tests/lib/rules/no-mutation-props.js b/tests/lib/rules/no-mutation-props.js
index 139136ab64..678796d5d6 100644
--- a/tests/lib/rules/no-mutation-props.js
+++ b/tests/lib/rules/no-mutation-props.js
@@ -1,6 +1,6 @@
/**
* @fileoverview Prevent mutation of this.props
- * @author Ian Schmitz
+ * @author Ian Schmitz, Joey Baker
*/
'use strict';
@@ -8,10 +8,10 @@
// Requirements
// ------------------------------------------------------------------------------
-var rule = require('../../../lib/rules/no-mutation-props');
-var RuleTester = require('eslint').RuleTester;
+const rule = require('../../../lib/rules/no-mutation-props');
+const RuleTester = require('eslint').RuleTester;
-var parserOptions = {
+const parserOptions = {
ecmaVersion: 6,
ecmaFeatures: {
jsx: true
@@ -24,7 +24,8 @@ require('babel-eslint');
// Tests
// ------------------------------------------------------------------------------
-var ruleTester = new RuleTester();
+const errorMessage = 'A component must never modify its own props.';
+const ruleTester = new RuleTester();
ruleTester.run('no-mutation-props', rule, {
valid: [{
@@ -63,6 +64,58 @@ ruleTester.run('no-mutation-props', rule, {
'}'
].join('\n'),
parserOptions: parserOptions
+ }, {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {list} = this.thing;',
+ ' list.push(1);',
+ ' this.thing.list.push(1);',
+ ' this.thing.foo.list.push(1);',
+ ' const foo = [];',
+ ' foo.push(1);',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions
+ }, {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {foo} = this.thing',
+ ' delete foo.bar;',
+ ' delete this.thing.foo',
+ ' const bar = {a: 1};',
+ ' delete bar.a;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions
+ }, {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const foo = {}',
+ ' Object.defineProperty(foo, "bar");',
+ ' Object.defineProperty(this.foo, "bar");',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions
+ }, {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {foo} = this.props',
+ ' Object.assign({}, foo, {bar: 1});',
+ ' Object.assign(this.foo, {foo: 1});',
+ ' Object.assign({}, this.props, {foo: 1});',
+ ' Object.assign({}, this.props.foo, {foo: 1});',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions
}],
invalid: [{
@@ -76,7 +129,7 @@ ruleTester.run('no-mutation-props', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{
- message: 'A component must never modify its own props.'
+ message: errorMessage
}]
}, {
code: [
@@ -89,7 +142,7 @@ ruleTester.run('no-mutation-props', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{
- message: 'A component must never modify its own props.'
+ message: errorMessage
}]
}, {
code: [
@@ -102,7 +155,7 @@ ruleTester.run('no-mutation-props', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{
- message: 'A component must never modify its own props.'
+ message: errorMessage
}]
}, {
code: [
@@ -116,13 +169,221 @@ ruleTester.run('no-mutation-props', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{
- message: 'A component must never modify its own props.',
+ message: errorMessage,
line: 3,
column: 5
}, {
- message: 'A component must never modify its own props.',
+ message: errorMessage,
+ line: 4,
+ column: 5
+ }]
+ },
+ {
+ code: [
+ 'class Hello extends React.Component {',
+ ' helper() {',
+ ' const {foo} = this.props;',
+ ' foo.bar = 1;',
+ ' }',
+ ' render() {',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions,
+ errors: [{
+ message: errorMessage,
+ line: 4,
+ column: 5
+ }]
+ },
+ {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {list} = this.props;',
+ ' list.push(1);',
+ ' list.pop();',
+ ' list.shift();',
+ ' list.unshift(1);',
+ ' this.props.foo.push(1);',
+ ' this.props.foo.pop();',
+ ' this.props.foo.shift();',
+ ' this.props.foo.unshift(1);',
+ ' this.props.foo.list.push(1);',
+ ' this.props.foo.list.pop();',
+ ' this.props.foo.list.shift();',
+ ' this.props.foo.list.unshift(1);',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions,
+ errors: [{
+ message: errorMessage,
+ line: 4,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 5,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 6,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 7,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 8,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 9,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 10,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 11,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 12,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 13,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 14,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 15,
+ column: 5
+ }]
+ },
+ {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {foo} = this.props;',
+ ' delete foo.bar;',
+ ' const [bar] = this.props.thing;',
+ ' delete bar.baz;',
+ ' const baz = this.props.baz',
+ ' delete baz.a;',
+ ' delete this.props.foo;',
+ ' delete this.props.foo.bar;',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions,
+ errors: [{
+ message: errorMessage,
+ line: 4,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 6,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 8,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 9,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 10,
+ column: 5
+ }]
+ },
+ {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {foo} = this.props;',
+ ' Object.defineProperty(foo, "bar");',
+ ' const [bar] = this.props.thing',
+ ' Object.defineProperty(bar, "baz");',
+ ' const baz = this.props.baz',
+ ' Object.defineProperty(baz, "thing");',
+ ' Object.defineProperty(this.props, "foo");',
+ ' Object.defineProperty(this.props.foo, "foo");',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions,
+ errors: [{
+ message: errorMessage,
line: 4,
column: 5
+ }, {
+ message: errorMessage,
+ line: 6,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 8,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 9,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 10,
+ column: 5
+ }]
+ },
+ {
+ code: [
+ 'class Hello extends React.Component {',
+ ' render() {',
+ ' const {foo} = this.props;',
+ ' Object.assign(foo, {bar: 1});',
+ ' const [bar] = this.props.thing;',
+ ' Object.assign(bar, {baz: 1});',
+ ' const baz = this.props.baz;',
+ ' Object.assign(baz, {bat: 1});',
+ ' Object.assign(this.props, {foo: 1});',
+ ' Object.assign(this.props.baz, {foo: 1});',
+ ' return ;',
+ ' }',
+ '}'
+ ].join('\n'),
+ parserOptions: parserOptions,
+ errors: [{
+ message: errorMessage,
+ line: 4,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 6,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 8,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 9,
+ column: 5
+ }, {
+ message: errorMessage,
+ line: 10,
+ column: 5
}]
}
/**
@@ -139,7 +400,7 @@ ruleTester.run('no-mutation-props', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{
- message: 'A component must never modify its own props.'
+ message: errorMessage
}]
}*/
]