Add no-mutation-props

[joeybaker]

Fixes #1113

This is based on the work in https://github.com/yannickcr/eslint-plugin-react/pull/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

[joeybaker]

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]

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 😄

https://github.com/yannickcr/eslint-plugin-react/pull/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]

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]

@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]

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!

[joeybaker]

@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.

[joeybaker]

@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]

I'm thinking a schema like this:

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

[joeybaker]

Interesting. A few questions:

1. Why enableInstanceMethods over allowArrayMutation? "Instance methods" seem a little vague to me.
2. Why bother with the separate arrays and booleans? Seems like if you provide an array, the boolean value is superfluous.
3. 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]

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]

Fair enough on the config option. Will change.

I really don't see the value in accommodating people who override the language builtins. Adding a config option will make the documentation a little more confusing, but your call. I'll go ahead and add it.

[joeybaker]

@ljharb options added, and all feedback addressed!

[joeybaker]

@ljharb Okay, Reflect mutations added. Where do we stand?

[joeybaker]

@ljharb 👋

[joeybaker]

I'm unsure of who else to ping, so @yannickcr @jseminck @jackyho112 what are your thoughts on https://github.com/yannickcr/eslint-plugin-react/pull/1416#discussion_r139571616

[joeybaker]

@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]

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]

@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]

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

[ljharb]

@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]

@joeybaker @ljharb any news on this?