jonaskello / tslint-immutable

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

Ban Object.assign mutation #112

Closed mmkal closed 5 years ago

mmkal commented 5 years ago

Could this library have an option to ban Object.assign?

It'd probably need to only apply to non-empty objects. i.e. this should be banned because it mutates a:

const a = { message: 'hello' }
const b = Object.assign(a, { message: 'bye' })
a.message // 'bye'
b.message // 'bye'

But this could be considered ok:

const a = { message: 'hello' }
const b = Object.assign({}, a, { message: 'bye' })
a.message // 'hello'
b.message // 'bye'
jonaskello commented 5 years ago

I think this sound like a good idea. The case of assigning to the empty object might be OK but perhaps we should also have the option to ban Object.assign altogether for cases where you want to promote object spread instead of empty object assign (const c = {...a, ...b}).

mmkal commented 5 years ago

Maybe the ignore-prefix pattern could work here by looking at the prefix of the arguments passed to Object.assign. So users who want to allow empty object assign can use "ignore-prefix":["{}"]. That would have the added benefit of allowing other use-cases like adding custom properties to functions, and if people want to ban it completely they just don't put anything in ignore-prefix.

RebeccaStevens commented 5 years ago

I feel this should be added to the no-object-mutation rule.

jonaskello commented 5 years ago

That makes sense since Object.assign does object mutation. In that case the option to enable Object.assign({}, a, b) would probably have to be something specific like ignore-empty-assign. It should probably be noted in the docs that const x = {..a, ...b} is a better way to do it, provided that your compiler supports it.

@mmkal Do you have any thoughts on putting this in the no-object-mutation rule?

mmkal commented 5 years ago

I can see the logic to that. It might be nice to be able to configure them separately, but maybe that could be handled by ignore-prefix.

Would it be possible to make the ignore-prefix whitelist apply to the Object.assign expression, which actually does the mutation, rather than the whole line?

e.g. with "ignore-prefix": ["Object.assign({}"]:

const foo = Object.assign({}, { msg: 'hello' })

would be ok (without needing "ignore-prefix": ["const foo"] or anything like that).

That way you wouldn't need a special flag like ignore-empty-assign, and it would allow easily disabling the ban completely, or whitelisting by naming conventions, etc.