Thoughts on "Never mutate parameters"

[AlicanC]

1- This rule's "Do not reassign parameters" part should be split into another rule, because reassigning a parameter isn't actually mutation.

Plus it sounds like no-param-reassign rule also covers mutation, but it doesn't. It would be nice to easily see what gets covered by ESLint and what doesn't.

---

2- Why shouldn't we reassign parameters?

Overwriting parameters can lead to unexpected behavior, especially when accessing the arguments object.

Accessing arguments is already discouraged. Should we really care about breaking something we aren't supposed to use? What are other unexpected behaviors can this cause?

[sandyjoshi]

2- In javascript objects are passed by reference, so when you pass object to any function and modify that object inside that function, it means the reference of object which is passed into a function will get affected. I think it is main "unexpected behaviors".

[AlicanC]

Reassigning parameters doesn't do that. Mutating parameters do that.

const myObj = {
  adele: 'hello',
};

function myFunc(myParam) {
  myParam = {};

  myParam.adele = 'rolling in the deep';
}

myFunc(myObj);

console.log(myObj); // { adele: 'hello' }

Mutating parameters is wrong, I have no objection to that rule.

[ljharb]

Reassigning parameters deoptimizes in many engines, specifically v8 - it's a horrible idea to do it. Variables are free, and creating new ones rather than reusing old ones makes code much clearer.

(In addition, nothing in JS is passed by reference, everything is passed by value, where objects are a form of "reference value" - this is a good read on the subject)

A PR would be welcome if there's verbage that could be cleared up.

[carpeliam]

Not sure if this is the place for additional thoughts, but I'm a little unsure as to how I should handle something like array.forEach((obj) => { obj.prop = true; });. Other than that, I'm a fan.

[ljharb]

@carpeliam by copying instead of mutating: array.map(obj => Object.assign({}, obj, { prop: true }))

[noelleleigh]

@ljharb I'm not sure that solution works in the case of setting the style attributes of a list of DOM nodes:

nodeArray.forEach(node => (node.style.display = 'none'));

What do you do when you have to mutate parameters in-place?

[ljharb]

@noahleigh yes, that's a good point - the DOM is one of the places that makes it very hard to avoid mutation (which is part of why we use React at Airbnb). For cases like these, I'd use an eslint override comment: // eslint-disable-line no-param-reassign

[emilgoldsmith]

Sorry to revive a very old and dead issue but I'm really curious about this. So this solution for arrays seems very good:

@carpeliam by copying instead of mutating: array.map(obj => Object.assign({}, obj, { prop: true }))

But what if it's an object of objects? Would you do something like this:

const mutatedObject = {}
oldObject.forEach((obj, key) => mutatedObject[key] = Object.assign({}, mutation))
oldObject = mutatedObject

or just disable the eslint rule or do you have a neater option?

Because for example using lodash's _.map which works on objects still returns an array, and that's of course not desireable when you want an object out.

[ljharb]

@emilgoldsmith you'd do something like:

const mutatedObject = Object.entries(oldObject).reduce((acc, [key, value]) => {
  return { ...acc, [key]: { ...value, ...mutation } };
}, {});

[emilgoldsmith]

Hmm interesting. Thanks for the answer!

[lukemcgregor]

In the context of this does anyone have thoughts on immer which clearly violates this rule (see immerjs/immer#189)

[noelzubin]

I don't see what the issue is with mutating variables. Having side effects in the code is definitely bad but mutating variables are perfectly fine.

[noelzubin]

The no-param-reassign rule in eslint is very confusing. And people mistake its purpose and what it tries to fix.

[noelzubin]

An ideal fix for this would be to create a linter that can find sideEffects.

[ljharb]

@noelzubin reassigning variables (variables can not be mutated, only values can be) definitely is not "perfectly fine" and can make code harder to follow.

While that would be nice, finding side effects statically is impossible in a dynamic language.

[tianzhich]

@ljharb Hi Jordan, you said Reassigning parameters deoptimizes in many engines, specifically v8 before, can you give some detailed explanations about that or some learning articles about that deoptimization in v8 engine.

[ljharb]

@tianzhich https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments, although that may no longer be true in TurboFan.

[ashwani-pandey]

This is an old, closed issue but still wanted to put my point here regarding how helpful this rule is, as I experienced it first-hand. It's the same one that I mentioned on another thread here - #719 (comment). Mentioning it here too as these two github threads are the top ones on google search while looking out to properly fix this issue.

I faced an issue with this, and my first thought was to simply put a disable command before that line. But I realized after a few moments how bad an idea it would have been.

This is what I was doing where I had a list of objects, and I needed to update each object by replacing the value of one of its properties and deleting another property.

// originalMembers is a list of objects.
const newMembers = [];
_.each(originalMembers, function(member) {
  member.attributes = [
    {
      'key': 'key1',
      'value': 'value1',
    },
    {
      'key': 'key2',
      'value': 'value2',
    }
  ]
  delete member.keyToDelete
  newMembers.push(member)
})

The new array comes as expected, but it modifies the original array too. Problem? This is part of a method with quite a few chained promises, and someone down the line can try an operation on the original array without realizing that it had been modified (unintentionally) somewhere in the code.

In my case, I finally rewrote the existing piece in the following way to completely avoid mutation.

const newMembers = _.map(originalMembers, ({ approved, ...member }) => ({
  ...member,
  attributes: [
    {
      key: 'key1',
      value: 'value1',
    },
    {
      key: 'key2',
      value: 'value2',
    },
  ],
}));

We should make changes as per the rule ( without compromising on performance ) before quickly jumping on to disable the rule. At the end, thanks a lot, @ljharb for this rule. Saves from a lot of future bugs :)