jfmengels / eslint-plugin-fp

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

Disallow array mutation #3

Closed amilajack closed 8 years ago

amilajack commented 8 years ago

Mutative array methods, such as push, are not disabled by this library yet. That would be a nice addition.

jfmengels commented 8 years ago

Hi @amilajack!

Yes, that is on my todo-list, but it's better to create an issue about this.

The problem with this rule, is that it is very hard in JavaScript (without tools like Flow/TypeScript) to know the type of a variable.

Tak the following example:

function pushStuff(variable, value) {
  return variable.push(value); // (1)
}

const res = pushStuff([1, 2, 3], 4); // (2)

function createVariable(value) {
  return {
    value: value,
    push: (newValue) => createVariable(value.concat(newValue))
  };
}
const res = pushStuff(createVariable([1, 2, 3]), 4); // (3)

In (1), this rule would probably report an error, because we're using push on a variable. When used in example (2), it proves to be a useful error, as there is indeed a use of a mutative push method. When used in example (3) however, the use of push is absolutely valid, non-mutative, FP, etc.

I think that this rule will probably simply disallow the use of every method named after a mutative array method. This will probably create quite a few false positives, but if someone has a better idea, I'm all ears!

amilajack commented 8 years ago

I think most users would probably be using the standard push method. The people that do something like the example you gave have the option of disabling the rule.

jfmengels commented 8 years ago

I think most users would probably be using the standard push method.

True, but I'd think that most users that would turn this rule on wouldn't :smile: Just hoping to find a solution that will lead to the least false positives possible, but don't think we can do much.

[They] have the option of disabling the rule.

Do you mean using eslint comments? Absolutely. Though I get the feeling that disabling a rule is usually used as a last resort, and that for most it is not something that you should find normal to do, which I understand absolutely. From what I see, in a lot of cases, users open issues and request a way to allow exceptions for XYZ. Maybe this rule won't be an exception :fingers_crossed:.

jfmengels commented 8 years ago

https://github.com/jfmengels/eslint-plugin-fp/blob/master/docs/rules/no-mutating-methods.md is out in the v2 release, let me know how it works out for you :)

amilajack commented 8 years ago

Thanks!