Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 13, 2019

Conversation

RebeccaStevens
Copy link
Collaborator

@RebeccaStevens RebeccaStevens commented Mar 12, 2019

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 });

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@RebeccaStevens
Copy link
Collaborator Author

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

@codecov
Copy link

codecov bot commented Mar 12, 2019

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%> (ø) ⬆️

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.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@jonaskello
Copy link
Owner

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
Copy link
Owner

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

Copy link
Owner

@jonaskello jonaskello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@RebeccaStevens RebeccaStevens changed the title Fixes #112 - Object.assign mutation is no longer allowed on identifiers. Fixes #112 - Object.assign mutation is no longer allowed on identifiers and property access expressions. Mar 13, 2019
@RebeccaStevens RebeccaStevens merged commit 479cab8 into master Mar 13, 2019
@RebeccaStevens RebeccaStevens deleted the fix/issue-112 branch March 13, 2019 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants