Add no-mutation-props #1416

joeybaker · 2017-09-07T15:48:06Z

This is based on the work in #1145 but reports on many more cases.

Now warns on:

array mutations
Object.assign
Object.defineProperty
delete
all of the above also works with array and object destructuring and
variable assignment

Fix change requests mentioned in jsx-eslint#1145

Fixes jsx-eslint#1113 Now warns on: * array mutations * `Object.assign` * `Object.defineProperty` * `delete` * all of the above also works with array and object destructuring and variable assignment

joeybaker · 2017-09-07T17:21:39Z

Once this is in a good place, it's probably a good idea to use the same logic in the no-direct-mutation-state rule.

jseminck · 2017-09-07T17:22:45Z

lib/rules/no-mutation-props.js

+          assignments.forEach(assignment => {
+            components.add(assignment);
+            components.set(assignment, {
+              isFromProps: true


I'm not sure how I feel about using the components as a storage for non-component nodes... Maybe we can use some local collection instead?

Fair enough, I was just trying to re-use the logic since what I need is all there. But, I'm happy to copy/paste what I need into here?

Something like:

assignments.forEach(assignment => { allAssignments.push(assignment.range.join(':')); // not sure about the name of the array :( });

And where it's checked:

const currentIdentifier = allAssignments.indexOf(originalIdentifiers[originalIdentifiers.length - 1].range.join(':')) >= 0; return currentIdentifier;

This way, at the end of the file there will only be the actual components inside the list, and we skip looping over all those assignments. For all tests, list would only contain 1 value.

Object.keys(list).forEach(key => { const component = list[key]; reportMutations(component); });

What do you think?

jseminck · 2017-09-07T17:30:31Z

Can you also add support for this.props.foo++? There's a PR that adds this to the no-direct-mutation-state rule, so I think we'll want it here too 😄

#1387

On the other hand, this rule supports other cases that no-direct-mutation-state supports, e.g. those array mutations. I'm not sure what would be a good way to share that code as you already mentioned.

joeybaker · 2017-09-07T17:48:18Z

Yea, ++ and -- are good calls. Can do.

I'd love to share code between this and no-direct-mutation-state. Perhaps the way to do that is to add isPropsNode and the new non-components set/get code to utils?

joeybaker · 2017-09-07T18:41:29Z

@jseminck changes made. I'd be happy to look at sharing code between this rule and no-direct-mutation-state, but I suggest we do that in a separate PR that focuses on improving no-direct-mutation-state.

jseminck · 2017-09-07T18:51:06Z

Yeah. I agree!

Looks good for me. It's a pleasure to read your code. 👍 Someone else should also still go over the code since I'm not an official maintainer though!

ljharb · 2017-09-07T20:53:59Z

docs/rules/no-mutation-props.md

+var Hello = React.createClass({
+  render: function() {
+    const {list} = this.props;
+    list.push(2);


i don't see how this can safely be considered a mutation; list could be anything with a "push" method, there's no way to know it's an array.

That's a great point. I'd hate to loose the functionality though. Any thoughts on how we could determine it's an array?

Meanwhile, on Gitter…

@not-an-aardvark suggests that leave this rule as-is since these method names tend to imply mutation anyway (also, it would be pretty weird to have a prop that isn't a plain object). Further, people can disable custom use-cases with an inline eslint-disable.

If we want to be very safe, we could put this behind an option flag?

I think putting it behind an option is a great compromise - what about two booleans, and two lists of 1) instance methods, and 2) static methods? That way the instance method list could default to ARRAY_MUTATIONS, and the static method list could default to ['Object.assign', 'Object.defineProperties', 'Object.defineProperty'] - and both booleans would default to false.

That way most people wouldn't touch the lists at all, they'd just enable the booleans - but those who wanted to update the lists could do so.

I'm with you on the array mutations, but I'm not sure I understand the danger of the static methods. Can you run that by me one more time? When is Object.assign(this.props, {…}) ever a safe operation?

ljharb · 2017-09-07T20:55:17Z

docs/rules/no-mutation-props.md

+
+var Hello = React.createClass({
+  render: function() {
+    Object.assign(this.props.foo, {bar: 'baz'})


If we're going to support builtins like Object.assign and Object.defineProperty (where's Object.defineProperties btw), we'd need to support a way to allow this to be extensible - in other words, to allow devs to specify a custom function name, and which argument(s) are mutated as well.

I don't think we should support functions in this way in the initial version of the rule.

I'm not sure I follow? Object.assign is never safe to use when this.props is the first argument, no?

Object.defineProperties is a good catch! Added.

ljharb · 2017-09-07T20:55:27Z

docs/rules/no-mutation-props.md

+
+var Hello = React.createClass({
+  render: function() {
+    Object.defineProperty(this.props, 'foo')


please ensure that all examples properly use semicolons.

ljharb · 2017-09-07T20:56:11Z

lib/rules/no-mutation-props.js

+'use strict';
+
+const Components = require('../util/Components');
+const ARRAY_MUTATIONS = ['push', 'pop', 'shift', 'unshift'];


although i don't think we can safely check for any of these, you're also missing the other array mutator methods: splice, reverse, sort, fill, copyWithin.

Added, but I'll hold of on adding tests until we resolve what to do here.

ljharb · 2017-09-07T20:56:47Z

lib/rules/no-mutation-props.js

+     */
+    const addToPropsNodes = node => {
+      const id = getId(node);
+      if (propsNodes[id]) {


if (!propsNodes[id]) { propsNodes[id] = node; } return propsNodes[id];

Make sense.

ljharb · 2017-09-07T20:57:11Z

lib/rules/no-mutation-props.js

+     * Gets a unique ID for a node
+     * @param {Object} node The node to get an ID for
+     */
+    const getId = node => node && node.range.join(':');


what happens if the range only has one item, and that item is toString?

I'm not sure I know what you mean? I didn't think it was possible for range to have only one item? This logic is copied from https://github.com/yannickcr/eslint-plugin-react/blob/f1e86b5ba32ed9359eb7823b98f7a1dcc52376e3/lib/util/Components.js#L43-L45

Fair, but since it's being used as an object key, without any prefix, there's still the possibility that it could conflict with an Object.prototype key.

Sorry, I'm still not following you. Will the AST allow for range to be anything other than a tuple of integers? We have full control of the object this key will be used for, so a string key composed of two joined integers should be totally safe, no?

Yes, I'm just not that familiar with what the AST could return. If range always contains > 1 item, then this is totally safe when node is truthy.

Yea, the spec says range must be an array with 2 integers: https://eslint.org/docs/developer-guide/working-with-plugins#all-nodes

ljharb · 2017-09-07T20:57:40Z

lib/rules/no-mutation-props.js

+     * @param {Object} node The node to add
+     */
+    const addToPropsNodes = node => {
+      const id = getId(node);


getId can return a falsy value - that would get coerced to a string here, and probably provide undesired results.

Sure? It would just try to find a falsey value key which would be undefined. I don't think there's any harm, but I agree it's safer and slightly faster to add a guard.

CR feedback 👍

per CR feedback

joeybaker · 2017-09-08T00:29:21Z

@ljharb I appreciate you taking a look. I'm sure you're busy, but I'd like to work with you to get this merged. I believe it's a nice feature. At the very least, it would help my team quite a bit.

ljharb · 2017-09-08T04:07:57Z

docs/rules/no-mutation-props.md

+var Hello = React.createClass({
+  render: function() {
+    const {list} = this.props;
+    list.push(2);


I think putting it behind an option is a great compromise - what about two booleans, and two lists of 1) instance methods, and 2) static methods? That way the instance method list could default to ARRAY_MUTATIONS, and the static method list could default to ['Object.assign', 'Object.defineProperties', 'Object.defineProperty'] - and both booleans would default to false.

That way most people wouldn't touch the lists at all, they'd just enable the booleans - but those who wanted to update the lists could do so.

ljharb · 2017-09-08T04:08:33Z

lib/rules/no-mutation-props.js

+     * Gets a unique ID for a node
+     * @param {Object} node The node to get an ID for
+     */
+    const getId = node => node && node.range.join(':');


Yes, I'm just not that familiar with what the AST could return. If range always contains > 1 item, then this is totally safe when node is truthy.

ljharb · 2017-09-08T04:18:26Z

lib/rules/no-mutation-props.js

+
+      let topObject = node;
+
+      // when looking at an object path, get the top-most object


this might be helpful to move to a helper method, so that this function's code can be simpler.

I hear you, but since this is only used in one place, I'm not sure the abstraction is worth it.

ljharb · 2017-09-08T04:22:02Z

lib/rules/no-mutation-props.js

+    function setPropMutation(mutation, node) {
+      const component = components.get(utils.getParentComponent());
+      const propMutations = component && component.propMutations
+        ? component.propMutations.concat([mutation])


is mutation an array? If not, the wrapping brackets aren't needed.

Nope! mutation is not an array, it a node.

ljharb · 2017-09-08T04:22:27Z

lib/rules/no-mutation-props.js

+
+    function setPropMutation(mutation, node) {
+      const component = components.get(utils.getParentComponent());
+      const propMutations = component && component.propMutations


Can we make component.propMutations always start out an empty array, so we don't need this check?

Could do, but that feels like unnecessary baggage across the rest of the rules if someone opts to not enable this rule. Thoughts?

I don't imagine that an empty array will slow things down much, compared the the speedup and clarity of not having to ever check for truthiness.

Just in case you've got a prop with array mutation methods on it and they're actually safe to use in a react component.

joeybaker · 2017-09-08T18:37:20Z

@ljharb okay, I've added an option for allowArrayMutation, but I'd love more clarification on when the static Object methods would ever be okay.

ljharb · 2017-09-09T05:52:02Z

I'm thinking a schema like this:

{
  "enableInstanceMethods": true,
  "instanceMethods": ["push", "shift"],
  "enableStaticMethods": true,
  "staticMethod": ["Object.assign", "Object.defineProperty"]
}

joeybaker · 2017-09-09T19:16:03Z

Interesting. A few questions:

Why enableInstanceMethods over allowArrayMutation? "Instance methods" seem a little vague to me.
Why bother with the separate arrays and booleans? Seems like if you provide an array, the boolean value is superfluous.
I'm still not clear on the danger of the object static methods. Could you give me an example when Object.assign(this.props, {}) would ever be safe?

ljharb · 2017-09-10T05:39:04Z

The value in providing both a boolean and an array is that a shared config (like the airbnb config) can specify the array without having to enable it, and users can enable it without having to copy/paste the array.

Object.assign is always safe when an object literal is the first argument :-) Regardless, Object.assign = function () { return 42; } would be totally safe and mutation-free.

joeybaker · 2017-09-19T03:44:09Z

I'm unsure of who else to ping, so @yannickcr @jseminck @jackyho112 what are your thoughts on #1416 (comment)

* Removes separate settings for Object and Reflect mutators * Adds lodash mutators as a default (we don't _have_ to do this, but I think it's very low risk, and we should)

ljharb · 2017-09-28T15:54:49Z

docs/rules/no-mutation-props.md


-When `true` the rule ignores [`Object` methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/object#Methods_of_the_Object_constructor) that can mutation props.
+By default, this is set to: `['Object.assign', 'Object.defineProperty', 'Object.defineProperties', 'Reflect.defineProperty', 'Reflect.deleteProperty', 'Reflect.set', '_.fill', '_.reverse', '_.assign', '_.extend', '_.assignIn', '_.assignInWith', '_.extendWith', '_.assignWith', '_.defaults', '_.defaultsDeep', '_.merge', '_.mergeWith', '_.set', '_.setWith']`. This contains language built-ins that can mutate objects (see: [`Object` methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/object#Methods_of_the_Object_constructor) and [`Reflect` mutation methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Reflects/Reflect#Methods)) as well as methods on [lodash](https://lodash.com) that will mutate either arrays or objects. You can override this with your own set of object/method pairs.


I think all the lodash/underscore methods should not be included by default; that's presuming that more people use those libs than do.

Tests should certainly ensure these would work, ofc :-)

I figured you'd say that, so I held off on tests ;)

Okay, I'll move them out, and just have them in the docs for easy reference.

joeybaker · 2017-09-28T16:55:58Z

@ljharb okay, options have been changed up and lodash methods removed. We now have just a disabledMethods option instead of 2 separate settings for each of Object and Reflect.

@yannickcr @jseminck @jackyho112 The githubs tell me that you're also maintainers on this package. I'd love to get your thoughts on disabling array mutations on this.props by default.

@ljharb makes the point that we can't do this with 100% safety. It's true, but I contend that the possibility of a prop that is not a plain object, that has methods named the same as array mutators, that in turn need to be called within a component is extremely limited. However, in the case it does occur, we've provide an option to disable that default.

We're at a bit of a deadlock, so any of you weighing in would be greatly appreciated. In general, disallowing this.props mutation would be really nice feature for my team (and I imagine many others) so I'd like to get this merged and released!

jseminck · 2017-09-28T17:39:24Z

I'm just a contributor 😄 I'll try to go through the whole PR tomorrow and see if I can weigh in, but honestly from following this from the side I have to admit some of the technical discussions are a bit above my technical knowledge level 😞

jackyho112 · 2017-09-29T01:31:11Z

@joeybaker

I am merely just a contributor as well. I am not sure if I will have time to review it this week, but I will try my best. 😄

joeybaker · 2017-09-29T16:04:50Z

@jseminck @jackyho112 whoops sorry about that! Opinions are very welcome though!

ljharb · 2021-08-09T06:10:21Z

@joeybaker it's been awhile, but if you're still interested in landing this, it'd be great if you rebased it and marked it as ready for review :-)

oteoe · 2022-08-25T07:53:44Z

@joeybaker @ljharb
any news on this?

ianschmitz added 2 commits September 7, 2017 08:22

no-mutation-props jsx-eslint#1113

63eaf60

Add no-mutation-props rule jsx-eslint#1113

a0a700e

Fix change requests mentioned in jsx-eslint#1145

joeybaker mentioned this pull request Sep 7, 2017

Add no-mutation-props rule #1145

Closed

joeybaker force-pushed the no-props-mutation branch 3 times, most recently from c653ea1 to 70ecbae Compare September 7, 2017 15:58

Add(no-mutation-props) catches way more things

d934305

Fixes jsx-eslint#1113 Now warns on: * array mutations * `Object.assign` * `Object.defineProperty` * `delete` * all of the above also works with array and object destructuring and variable assignment

joeybaker force-pushed the no-props-mutation branch from 70ecbae to d934305 Compare September 7, 2017 16:18

jseminck reviewed Sep 7, 2017

View reviewed changes

joeybaker added 2 commits September 7, 2017 10:59

Fix(no-mutation-props) don't mis-use components

e104ca3

Fix(no-mutation-props) disallow update experssions

3d8001e

ljharb added the new rule label Sep 7, 2017

ljharb requested changes Sep 7, 2017

View reviewed changes

joeybaker added 4 commits September 7, 2017 17:06

Doc(no-mutation-props) fix semis

b0dc5ad

Refactor(no-mutation-props) clean addToPropsNodes

16ae29d

CR feedback 👍

Add(no-mutations-props) add Object.defineProperies

16bbce3

Fix(no-mutation-props) add more array mutations

95e73d9

per CR feedback

ljharb reviewed Sep 8, 2017

View reviewed changes

jseminck mentioned this pull request Sep 8, 2017

react/no-direct-mutation-state doesn't catch all cases #1418

Open

Add(no-mutation-props) option: allowArrayMutation

75d93a2

Just in case you've got a prop with array mutation methods on it and they're actually safe to use in a react component.

yannickcr force-pushed the master branch from c8dedeb to dc9f85e Compare September 24, 2017 18:16

Change(no-mutation-props) use disabledMethods

3a90f8b

* Removes separate settings for Object and Reflect mutators * Adds lodash mutators as a default (we don't _have_ to do this, but I think it's very low risk, and we should)

ljharb requested changes Sep 28, 2017

View reviewed changes

Change(no-mutation-props) rm lodash methods

df623c4

yannickcr force-pushed the master branch from dca5cd5 to c148893 Compare November 18, 2017 17:26

ljharb mentioned this pull request Oct 23, 2018

Rule idea: no-mutation-props #1113

Open

ljharb added the enhancement label Apr 12, 2019

ljharb marked this pull request as draft August 9, 2021 06:10

ljharb force-pushed the master branch from 0d1a62e to f7fe38f Compare July 11, 2022 15:02

ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45

ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19

ljharb force-pushed the master branch from 57bb8d1 to ae64aa8 Compare June 8, 2023 22:37

ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25

ljharb force-pushed the master branch from 301a48d to 2c0b10c Compare December 9, 2024 20:26


		let topObject = node;

		// when looking at an object path, get the top-most object


		When `true` the rule ignores [`Object` methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/object#Methods_of_the_Object_constructor) that can mutation props.
		By default, this is set to: `['Object.assign', 'Object.defineProperty', 'Object.defineProperties', 'Reflect.defineProperty', 'Reflect.deleteProperty', 'Reflect.set', '_.fill', '_.reverse', '_.assign', '_.extend', '_.assignIn', '_.assignInWith', '_.extendWith', '_.assignWith', '_.defaults', '_.defaultsDeep', '_.merge', '_.mergeWith', '_.set', '_.setWith']`. This contains language built-ins that can mutate objects (see: [`Object` methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/object#Methods_of_the_Object_constructor) and [`Reflect` mutation methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Reflects/Reflect#Methods)) as well as methods on [lodash](https://lodash.com) that will mutate either arrays or objects. You can override this with your own set of object/method pairs.

Add no-mutation-props #1416

Are you sure you want to change the base?

Add no-mutation-props #1416

Conversation

joeybaker commented Sep 7, 2017

joeybaker commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

jseminck commented Sep 7, 2017 • edited Loading

joeybaker commented Sep 7, 2017

joeybaker commented Sep 7, 2017

jseminck commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeybaker Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeybaker commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeybaker Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeybaker commented Sep 8, 2017

ljharb commented Sep 9, 2017

joeybaker commented Sep 9, 2017 • edited Loading

ljharb commented Sep 10, 2017

joeybaker commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeybaker commented Sep 28, 2017

jseminck commented Sep 28, 2017

jackyho112 commented Sep 29, 2017

joeybaker commented Sep 29, 2017

ljharb commented Aug 9, 2021

oteoe commented Aug 25, 2022

jseminck Sep 7, 2017 •

edited

Loading

jseminck commented Sep 7, 2017 •

edited

Loading

joeybaker Sep 8, 2017 •

edited

Loading

joeybaker Sep 8, 2017 •

edited

Loading

joeybaker commented Sep 9, 2017 •

edited

Loading