Skip to content

Commit c8f2bf5

Browse files
sindresorhussindresorhus
authored
no-useless-spread: Check object spread in Object.assign() (#3188)
1 parent fc4e29a commit c8f2bf5

6 files changed

Lines changed: 309 additions & 33 deletions

File tree

‎docs/rules/no-useless-spread.md‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
💼 This rule is enabled in the following [configs](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config): ✅ `recommended`, ☑️ `unopinionated`.
66

7-
🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).
7+
🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/latest/use/core-concepts#rule-suggestions).
88

99
<!-- end auto-generated rule header -->
1010
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->
@@ -14,6 +14,7 @@
1414
- Spread an array literal as elements of an array literal
1515
- Spread an array literal as arguments of a call or a `new` call
1616
- Spread an object literal as properties of an object literal
17+
- Use an object literal with only spread properties as an `Object.assign(…)` source argument
1718
- Spread an iterable as the only argument to a collection constructor that accepts a single iterable argument
1819
- Use spread syntax to clone an array created inline
1920

@@ -29,7 +30,7 @@
2930
- `Promise.{all,allSettled,any,race}(…)`
3031
- `Object.fromEntries(…)`
3132

32-
- `for…of` loop can iterate over any iterable object not just array, so it's unnecessary to convert the iterable to an array.
33+
- `for…of` loop can iterate over any iterable object not just arrays, so it's unnecessary to convert the iterable to an array.
3334

3435
- `yield*` can delegate to another iterable, so it's unnecessary to convert the iterable to an array.
3536

@@ -67,6 +68,16 @@ const object = new Foo(firstArgument, ...[secondArgument], thirdArgument);
6768
const object = new Foo(firstArgument, secondArgument, thirdArgument);
6869
```
6970

71+
```js
72+
// ❌
73+
Object.assign(target, {...source});
74+
75+
// 💡
76+
Object.assign(target, source);
77+
```
78+
79+
The `Object.assign(target, {...source})` case is intentionally suggested instead of autofixed because replacing the temporary object can change observable getter and setter timing.
80+
7081
```js
7182
// ❌
7283
const set = new Set([...iterable]);

‎readme.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ export default [
178178
| [no-useless-length-check](docs/rules/no-useless-length-check.md) | Disallow useless array length check. | ✅ ☑️ | 🔧 | |
179179
| [no-useless-promise-resolve-reject](docs/rules/no-useless-promise-resolve-reject.md) | Disallow returning/yielding `Promise.resolve/reject()` in async functions or promise callbacks | ✅ ☑️ | 🔧 | |
180180
| [no-useless-recursion](docs/rules/no-useless-recursion.md) | Disallow simple recursive function calls that can be replaced with a loop. | ✅ | | |
181-
| [no-useless-spread](docs/rules/no-useless-spread.md) | Disallow unnecessary spread. | ✅ ☑️ | 🔧 | |
181+
| [no-useless-spread](docs/rules/no-useless-spread.md) | Disallow unnecessary spread. | ✅ ☑️ | 🔧 | 💡 |
182182
| [no-useless-switch-case](docs/rules/no-useless-switch-case.md) | Disallow useless case in switch statements. | ✅ ☑️ | | 💡 |
183183
| [no-useless-undefined](docs/rules/no-useless-undefined.md) | Disallow useless `undefined`. | ✅ ☑️ | 🔧 | |
184184
| [no-zero-fractions](docs/rules/no-zero-fractions.md) | Disallow number literals with zero fractions or dangling dots. | ✅ ☑️ | 🔧 | |

‎rules/no-useless-spread.js‎

Lines changed: 69 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
isKnownNonArray,
77
isParenthesized,
88
isOnSameLine,
9+
getParenthesizedText,
910
} from './utils/index.js';
1011
import {
1112
isNewExpression,
@@ -20,14 +21,18 @@ const ITERABLE_TO_ARRAY = 'iterable-to-array';
2021
const ITERABLE_TO_ARRAY_IN_FOR_OF = 'iterable-to-array-in-for-of';
2122
const ITERABLE_TO_ARRAY_IN_YIELD_STAR = 'iterable-to-array-in-yield-star';
2223
const SPREAD_IN_COLLECTION_CONSTRUCTOR = 'spread-in-collection-constructor';
24+
const SPREAD_IN_OBJECT_ASSIGN = 'spread-in-object-assign';
2325
const CLONE_ARRAY = 'clone-array';
26+
const SUGGESTION_REMOVE_OBJECT_ASSIGN_SPREAD = 'suggestion/remove-object-assign-spread';
2427
const messages = {
2528
[SPREAD_IN_LIST]: 'Spread an {{argumentType}} literal in {{parentDescription}} is unnecessary.',
26-
[ITERABLE_TO_ARRAY]: '`{{parentDescription}}` accepts iterable as argument, it\'s unnecessary to convert to an array.',
27-
[ITERABLE_TO_ARRAY_IN_FOR_OF]: '`for…of` can iterate over iterable, it\'s unnecessary to convert to an array.',
28-
[ITERABLE_TO_ARRAY_IN_YIELD_STAR]: '`yield*` can delegate iterable, it\'s unnecessary to convert to an array.',
29+
[ITERABLE_TO_ARRAY]: '`{{parentDescription}}` accepts an iterable as an argument, it\'s unnecessary to convert to an array.',
30+
[ITERABLE_TO_ARRAY_IN_FOR_OF]: '`for…of` can iterate over an iterable, it\'s unnecessary to convert to an array.',
31+
[ITERABLE_TO_ARRAY_IN_YIELD_STAR]: '`yield*` can delegate to an iterable, it\'s unnecessary to convert to an array.',
2932
[SPREAD_IN_COLLECTION_CONSTRUCTOR]: '`{{constructorName}}` accepts a single iterable argument, spreading is misleading.',
33+
[SPREAD_IN_OBJECT_ASSIGN]: '`Object.assign(…)` source object with only spread properties is unnecessary.',
3034
[CLONE_ARRAY]: 'Unnecessarily cloning an array.',
35+
[SUGGESTION_REMOVE_OBJECT_ASSIGN_SPREAD]: 'Remove the object literal wrapper.',
3136
};
3237

3338
const collectionConstructors = ['Map', 'WeakMap', 'Set', 'WeakSet'];
@@ -53,6 +58,18 @@ const isSingleArraySpread = node =>
5358
&& node.elements.length === 1
5459
&& node.elements[0]?.type === 'SpreadElement';
5560

61+
const isObjectExpressionWithOnlySpreadProperties = node =>
62+
node.type === 'ObjectExpression'
63+
&& node.properties.length > 0
64+
&& node.properties.every(property =>
65+
property.type === 'SpreadElement'
66+
&& property.argument.type !== 'ObjectExpression');
67+
68+
const getObjectAssignReplacementText = (objectExpression, context) =>
69+
objectExpression.properties
70+
.map(property => getParenthesizedText(property.argument, context))
71+
.join(', ');
72+
5673
const parentDescriptions = {
5774
ArrayExpression: 'array literal',
5875
ObjectExpression: 'object literal',
@@ -341,6 +358,54 @@ const create = context => {
341358
};
342359
});
343360

361+
// Object.assign() source object spread
362+
context.on('ObjectExpression', objectExpression => {
363+
if (!isObjectExpressionWithOnlySpreadProperties(objectExpression)) {
364+
return;
365+
}
366+
367+
const callExpression = objectExpression.parent;
368+
if (!isMethodCall(callExpression, {
369+
object: 'Object',
370+
method: 'assign',
371+
minimumArguments: 2,
372+
optionalMember: false,
373+
optionalCall: false,
374+
})) {
375+
return;
376+
}
377+
378+
const argumentIndex = callExpression.arguments.indexOf(objectExpression);
379+
const hasGuaranteedTarget = callExpression.arguments
380+
.slice(0, argumentIndex)
381+
.some(argument => argument.type !== 'SpreadElement');
382+
if (
383+
argumentIndex <= 0
384+
|| !hasGuaranteedTarget
385+
) {
386+
return;
387+
}
388+
389+
const problem = {
390+
node: sourceCode.getFirstToken(objectExpression.properties[0]),
391+
messageId: SPREAD_IN_OBJECT_ASSIGN,
392+
};
393+
394+
if (sourceCode.getCommentsInside(objectExpression).length > 0) {
395+
return problem;
396+
}
397+
398+
return {
399+
...problem,
400+
suggest: [
401+
{
402+
messageId: SUGGESTION_REMOVE_OBJECT_ASSIGN_SPREAD,
403+
fix: fixer => fixer.replaceText(objectExpression, getObjectAssignReplacementText(objectExpression, context)),
404+
},
405+
],
406+
};
407+
});
408+
344409
// Spread in collection constructor
345410
context.on('NewExpression', node => {
346411
if (
@@ -479,6 +544,7 @@ const config = {
479544
recommended: 'unopinionated',
480545
},
481546
fixable: 'code',
547+
hasSuggestions: true,
482548
messages,
483549
languages: [
484550
'js/js',

‎test/no-useless-spread.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,37 @@ test.snapshot({
155155
'Promise.all(...[...iterable])',
156156
'new Map(...[...iterable])',
157157
'new Set(...[iterable])',
158+
159+
// Handled by the generic object literal spread fix, not the `Object.assign()` source suggestion.
160+
'Object.assign(target, {...{a}})',
161+
],
162+
});
163+
164+
// Object.assign() source object spread
165+
test.snapshot({
166+
valid: [
167+
'Object.assign(target, source)',
168+
'Object.assign(target, {})',
169+
'Object.assign(target, {foo})',
170+
'Object.assign(target, {foo, ...source})',
171+
'Object.assign(target, {...source, foo})',
172+
'Object.assign?.(target, {...source})',
173+
'Object?.assign(target, {...source})',
174+
'Object[assign](target, {...source})',
175+
'NotObject.assign(target, {...source})',
176+
'Object.assign({...source})',
177+
'Object.assign({...target}, source)',
178+
'Object.assign(...args, {...source})',
179+
],
180+
invalid: [
181+
'Object.assign(target, {...source})',
182+
'Object.assign(target, first, {...second})',
183+
'Object.assign(...args, target, {...source})',
184+
'Object.assign(target, {...first, ...second}, third)',
185+
'Object.assign(target, {...source,}, third)',
186+
'Object.assign(target, {...(( source ))})',
187+
'Object.assign(target, {...(foo, bar)})',
188+
'Object.assign(target, {/* keep */ ...source})',
158189
],
159190
});
160191

0 commit comments

Comments
 (0)