jfmengels / eslint-plugin-fp

ESLint rules for functional programming
MIT License
970 stars 36 forks source link

Allow `this` modification in `no-mutation` #1

Closed jfmengels closed 8 years ago

jfmengels commented 8 years ago

As proposed by @scottnonnenberg in https://github.com/jhusain/eslint-plugin-immutable/pull/15, the no-mutation rule could accept mutations to this, as they may make sense in some applications.

To do: Accept mutation of the this object, only if a corresponding flag is set.

dtinth commented 8 years ago

Maybe this should have an option to allow this just inside constructors (like how final fields work in Java). 😄

jfmengels commented 8 years ago

Hmm, not sure it makes sense. If you only allow this to be mutated in the constructor, then you don't really need it, and you can do it with closures.

function Person(age, firstName, lastName) {
  const isOver18 = age > 18;
  return {
    age: age,
    fullName: function() {
      return firstName + ' ' + lastName;
    }
  };
}

If you have an other use-case for this, I'm happy to hear it :)

scottnonnenberg commented 8 years ago

As you guessed on the original issue, I wanted support for this for React apps. Without it, as soon as you need to keep hold of a ref for some reason, you resort to eslint-disable. If it's not that common, maybe those exceptions are adequate.

jfmengels commented 8 years ago

@scottnonnenberg I haven't used React for a while, and haven't got far enough as to play with refs. Do you mind giving me an example of that use-case?

scottnonnenberg commented 8 years ago

Straight from my GatsbyJS-based blog:

  componentDidMount() {
    catchLinks(this.parentNode, href => this.context.router.push(href));
  }

  getParent(ref) {
    this.parentNode = ref; // eslint-disable-line
  }

  render() {
    return <div ref={this.getParent} />;
  }
jfmengels commented 8 years ago

Thanks!

Ok, I'll add a this exception behind a flag. By the way, you should try to avoid using eslint-disable-line without specifying the rules to ignore, as you might hide useful reports. See https://github.com/sindresorhus/eslint-plugin-xo/blob/master/docs/rules/no-abusive-eslint-disable.md :)

jfmengels commented 8 years ago

Added in 69982adfeec102646f9a8b2479e06df068eb06ef, and landed in 1.3.0. Thanks folks!

deepu105 commented 7 years ago

@jfmengels I was about to open a new issue then I came across this thread, I found this plugin useful and I guess being able to configure mutation of this only inside constructor is somthing thats useful in react apps especially, as you woul often assign this.state = .. in constructor but you wouldnt want to do that outside the constructor as state shouldnt be mutated. so somethink like below might be bit more flexible

"fp/no-mutation": ["error", {
  "commonjs": true,
  "allowThis": true,
  "allowThisInConstructor": true,
  "exceptions": [
    {"object": "foo", "property": "bar"}
  ]
}]

or even better would be if we can just specify method in the exceptions

"fp/no-mutation": ["error", {
  "commonjs": true,
  "allowThis": true,
  "exceptions": [
    {"object": "this", "property": "state", "method": "constructor"}
  ]
}]