Rule prefer-object-spread should not work when the first argument of Object.assign is not {}

[zheeeng]

Bug Report

• TSLint version: 5.7.0
• TypeScript version: 2.5
• Running TSLint via: VS Code

TypeScript code being linted

const values = [2, 3, 4, 5]
const obj = values.reduce(
    (o, v, i) => Object.assign(o, {[i]: v}),
    {},
)

with tslint.json configuration:

"prefer-object-spread": true,

Actual behavior

[tslint] 'Object.assign' returns the first argument. Prefer object spread if you want a new object. (prefer-object-spread)

Expected behavior

I actually want assign properties to the first argument, I want this side effects, it is common in reducing. The reasonable tips appearing is using Object.assign({}, o1, o2), it should be suggested to using spread instead of. What I'm using is the Object.assign designed for.

[ajafff]

While I agree that your code is valid, the rule is still right in this case. You can simply convert your code as follows:

const values = [2, 3, 4, 5]
const obj = values.reduce(
    (o, v, i) => ({...o, [i]: v}),
    {},
)

It yields the same result. In some other cases it even provides better type inference and checking. That's why the rule suggests object spread in the first place.

---

Regarding your feature request there are two possible options:

• add an option to turn off the extensive check if the first parameter is not an object literal
• change the rule to only do the basic check by default and add an option to enable the extensive check

[zheeeng]

If we eventually will merge all properties, why we have to spread properties out?

Moreover, if we wanna try such a meta programming:

const o = {
    a: 42
}

Object.defineProperty(o, 'a', {
    set(value) {
        console.log(value)
    }
})

// A
const oo = Object.assign(o, { a: 7 })
// B
const ooo = Object.assign({}, o, { a: 7 })
// C
const oooo = { ...o, ...{ a: 7 }}

Object.assign will trigger the setter and spread operator won't. Statement A print the '7', B and C have the same behavior. I think C shouldn't be a replacement for A but B.

[princjef]

It seems like there are two things to consider:

1. Whether there are cases where the transformation applied by the fixer changes the runtime behavior, possibly introducing a bug. I would argue yes because of the case where someone intentionally assigns into the first parameter.

2. Whether people should be allowed to disable the rule entirely in cases where the first parameter is not an object literal. I would argue yes for this as well, though I'd be inclined to take the approach in @ajafff's first option to keep the default behavior the same and allow people to make a conscious decision that they want to ignore that case.

[glen-84]

Since Object.assign modifies the first argument, wouldn't the fixer potentially break code?

var o1 = { a: 1 };
var o2 = { b: 2 };
var o3 = { c: 3 };

var obj = Object.assign(o1, o2, o3);
console.log(obj); // { a: 1, b: 2, c: 3 }
console.log(o1);  // { a: 1, b: 2, c: 3 }, target object itself is changed.

if (o1.b === 2) {
    // this would no longer run if Object.assign is changed to `{...o1, ...o2, ...o3}`?
}

If that's correct, I definitely think that the rule should only trigger when the first argument is a literal. In fact this could be considered a bug.

[steabert]

It seems there is a bug with auto-fixing Object.assign, e.g.:

const arr = ['a', 'b', 'c']
const arrMod = Object.assign([], arr, {1: 'd'})
Array.isArray(arrMod)

which will return true (still an Array)

Will be autofixed as:

const arr = ['a', 'b', 'c']
const arrMod = {...[], ...arr,  1: 'd'}
Array.isArray(arrMod)

which will return false (not an Array)

[adidahiya]

I agree that the rule is a bit aggressive right now. It applies unsafe auto fixes (related: #2625). I agree with @ajafff's suggestion of

change the rule to only do the basic check by default and add an option to enable the extensive check

This would align with the ESLint rule. The new rule option could be called "always", to match some of the other rule options of this flavor.

[RuLeZzz1987]

One more scenario when rule works incorrect

class Foo {
    handleBar() {
        console.log('bar handled');
    }
}

const foo = new Foo();


const bar: any = Object.assign(foo, {prop: 'ok'}); // <-- tslint suggest to replace with spread but it dramatically changes semantic

bar.handleBar();

[thorn0]

It's definitely a bug, not an enhancement. Please change the label.

change the rule to only do the basic check by default and add an option to enable the extensive check

This would align with the ESLint rule. The new rule option could be called "always", to match some of the other rule options of this flavor.

@adidahiya How exactly would it align? The ESLint rule doesn't have any options.

[adidahiya]

I'm suggesting that the default behavior of the rule (the proposed "basic" check) should work the same as the ESLint rule. not talking about configuration there

[JoshuaKGoldberg]

Removing Ignoring the Type: Breaking Change label per #4811. Now accepting PRs!

[JoshuaKGoldberg]

💀 It's time! 💀

TSLint is deprecated and no longer accepting pull requests other than security fixes. See #4534. ☠️
We recommend you instead use typescript-eslint to lint your TypeScript code with ESLint. ✅

👋 It was a pleasure open sourcing with you!

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 (#4534) and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋