jonaskello / tslint-immutable

TSLint rules to disable mutation in TypeScript.
MIT License
417 stars 14 forks source link

Fixes #112 - Object.assign mutation is no longer allowed on identifiers and property access expressions. #127

Closed RebeccaStevens closed 5 years ago

RebeccaStevens commented 5 years ago

Fixes #112

This PR makes it so Object.assign is banned when used with an identifier or a property access expressions as the first parameter.
For example, this would no longer be allowed:

const value = { msg1: "hello" };
const foo = Object.assign(value, { msg2: "world" });

But these are allowed:

const foo = Object.assign({}, { msg: "hello world" });
const foo = Object.assign({}, value, { msg2: "world" });
const foo = Object.assign({ ...value }, { msg2: "world" });

This is allowed too:

const bar = (a, b, c) => ({a, b, c});
const foo = Object.assign(bar(1, 2, 3), { d: 4 });
RebeccaStevens commented 5 years ago

@jonaskello Not sure if this is a breaking change or not. Type information is now need for the no-object-mutation rule.

codecov[bot] commented 5 years ago

Codecov Report

Merging #127 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #127   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          16     16           
  Lines         365    379   +14     
  Branches      154    165   +11     
=====================================
+ Hits          365    379   +14
Impacted Files Coverage Δ
src/noObjectMutationRule.ts 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a8124be...3365a12. Read the comment docs.

jonaskello commented 5 years ago

Well, that is a good question :-). I don't use any rules that require type checking myself so I'm probably not the best judge of this. From what I understand from the docs you need to specify an additional parameter to the tslint CLI so in that sense it could be considered breaking existing behaviour. On the other hand the purpose of the rule itself and it's options have no breaking changes so in that sense it is non-breaking. My initial feeling is that it could be a minor version but version numbers does not cost anything so there is really no rational reason that I can think of to not make it a major version :-).

Is there a way to make the rule gracefully fallback to reduced function when there is no typeinfo? In that case it would certainly be a minor version. But perhaps that would be strange behaviour and hard to document.

jonaskello commented 5 years ago

To sum up, you can make it minor or major I have no real preference :-).