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
New: Adds prefer-object-spread rule (refs: #7230) #9955
Changes from all commits
c0b3ec2
c8e0f9f
1c032db
4db39db
3819ad4
e39fd2d
7a7960a
0a0e1b1
751774d
afcacaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Prefer use of an object spread over `Object.assign` (prefer-object-spread) | ||
|
||
When Object.assign is called using an object literal as the first argument, this rule requires using the object spread syntax instead. This rule also warns on cases where an `Object.assign` call is made using a single argument that is an object literal, in this case, the `Object.assign` call is not needed. | ||
|
||
## Rule Details | ||
|
||
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
|
||
Object.assign({}, foo) | ||
|
||
Object.assign({}, {foo: 'bar'}) | ||
|
||
Object.assign({ foo: 'bar'}, baz) | ||
|
||
Object.assign({ foo: 'bar' }, Object.assign({ bar: 'foo' })) | ||
|
||
Object.assign({}, { foo, bar, baz }) | ||
|
||
Object.assign({}, { ...baz }) | ||
|
||
// Object.assign with a single argument that is an object literal | ||
Object.assign({}); | ||
|
||
Object.assign({ foo: bar }); | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
|
||
Object.assign(...foo); | ||
|
||
// Any Object.assign call without an object literal as the first argument | ||
Object.assign(foo, { bar: baz }); | ||
|
||
Object.assign(foo, Object.assign(bar)); | ||
|
||
Object.assign(foo, { bar, baz }) | ||
|
||
Object.assign(foo, { ...baz }); | ||
``` | ||
|
||
## When Not To Use It | ||
|
||
When you don't care about syntactic sugar added by the object spread property. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,278 @@ | ||
/** | ||
* @fileoverview Prefers object spread property over Object.assign | ||
* @author Sharmila Jesupaul | ||
* See LICENSE file in root directory for full license. | ||
*/ | ||
|
||
"use strict"; | ||
|
||
const matchAll = require("string.prototype.matchall"); | ||
|
||
/** | ||
* Helper that checks if the node is an Object.assign call | ||
* @param {ASTNode} node - The node that the rule warns on | ||
* @returns {boolean} - Returns true if the node is an Object.assign call | ||
*/ | ||
function isObjectAssign(node) { | ||
return ( | ||
node.callee && | ||
node.callee.type === "MemberExpression" && | ||
node.callee.object.name === "Object" && | ||
node.callee.property.name === "assign" | ||
); | ||
} | ||
|
||
/** | ||
* Helper that checks if the node needs parentheses to be valid JS. | ||
* The default is to wrap the node in parentheses to avoid parsing errors. | ||
* @param {ASTNode} node - The node that the rule warns on | ||
* @returns {boolean} - Returns true if the node needs parentheses | ||
*/ | ||
function needsParens(node) { | ||
const parent = node.parent; | ||
|
||
if (!parent || !node.type) { | ||
return true; | ||
} | ||
|
||
switch (parent.type) { | ||
case "VariableDeclarator": | ||
case "ArrayExpression": | ||
case "ReturnStatement": | ||
case "CallExpression": | ||
case "Property": | ||
return false; | ||
default: | ||
return true; | ||
} | ||
} | ||
|
||
/** | ||
* Determines if an argument needs parentheses. The default is to not add parens. | ||
* @param {ASTNode} node - The node to be checked. | ||
* @returns {boolean} True if the node needs parentheses | ||
*/ | ||
function argNeedsParens(node) { | ||
if (!node.type) { | ||
return false; | ||
} | ||
|
||
switch (node.type) { | ||
case "AssignmentExpression": | ||
case "ArrowFunctionExpression": | ||
case "ConditionalExpression": | ||
return true; | ||
default: | ||
return false; | ||
} | ||
} | ||
|
||
/** | ||
* Helper that adds a comma after the last non-whitespace character that is not a part of a comment. | ||
* @param {string} formattedArg - String of argument text | ||
* @param {array} comments - comments inside the argument | ||
* @returns {string} - argument with comma at the end of it | ||
*/ | ||
function addComma(formattedArg, comments) { | ||
const nonWhitespaceCharacterRegex = /[^\s\\]/g; | ||
const nonWhitespaceCharacters = Array.from(matchAll(formattedArg, nonWhitespaceCharacterRegex)); | ||
const commentRanges = comments.map(comment => comment.range); | ||
const validWhitespaceMatches = []; | ||
|
||
// Create a list of indexes where non-whitespace characters exist. | ||
nonWhitespaceCharacters.forEach(match => { | ||
const insertIndex = match.index + match[0].length; | ||
|
||
if (!commentRanges.length) { | ||
validWhitespaceMatches.push(insertIndex); | ||
} | ||
|
||
// If comment ranges are found make sure that the non whitespace characters are not part of the comment. | ||
commentRanges.forEach(arr => { | ||
const commentStart = arr[0]; | ||
const commentEnd = arr[1]; | ||
|
||
if (insertIndex < commentStart || insertIndex > commentEnd) { | ||
validWhitespaceMatches.push(insertIndex); | ||
} | ||
}); | ||
}); | ||
const insertPos = Math.max(...validWhitespaceMatches); | ||
const regex = new RegExp(`^((?:.|[^/s/S]){${insertPos}}) *`); | ||
|
||
return formattedArg.replace(regex, "$1, "); | ||
} | ||
|
||
/** | ||
* Helper formats an argument by either removing curlies or adding a spread operator | ||
* @param {ASTNode|null} arg - ast node representing argument to format | ||
* @param {boolean} isLast - true if on the last element of the array | ||
* @param {Object} sourceCode - in context sourcecode object | ||
* @param {array} comments - comments inside checked node | ||
* @returns {string} - formatted argument | ||
*/ | ||
function formatArg(arg, isLast, sourceCode, comments) { | ||
const text = sourceCode.getText(arg); | ||
const parens = argNeedsParens(arg); | ||
const spread = arg.type === "SpreadElement" ? "" : "..."; | ||
|
||
if (arg.type === "ObjectExpression" && arg.properties.length === 0) { | ||
return ""; | ||
} | ||
|
||
if (arg.type === "ObjectExpression") { | ||
|
||
/** | ||
* This regex finds the opening curly brace and any following spaces and replaces it with whatever | ||
* exists before the curly brace. It also does the same for the closing curly brace. This is to avoid | ||
* having multiple spaces around the object expression depending on how the object properties are spaced. | ||
*/ | ||
const formattedObjectLiteral = text.replace(/^(.*){ */, "$1").replace(/ *}([^}]*)$/, "$1"); | ||
|
||
return isLast ? formattedObjectLiteral : addComma(formattedObjectLiteral, comments); | ||
} | ||
|
||
if (isLast) { | ||
return parens ? `${spread}(${text})` : `${spread}${text}`; | ||
} | ||
|
||
return parens ? addComma(`${spread}(${text})`, comments) : `${spread}${addComma(text, comments)}`; | ||
} | ||
|
||
/** | ||
* Autofixes the Object.assign call to use an object spread instead. | ||
* @param {ASTNode|null} node - The node that the rule warns on, i.e. the Object.assign call | ||
* @param {string} sourceCode - sourceCode of the Object.assign call | ||
* @returns {Function} autofixer - replaces the Object.assign with a spread object. | ||
*/ | ||
function autofixSpread(node, sourceCode) { | ||
return fixer => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this might be able to simplify with generator function. (It's just a impression) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you give me an example of this? I'm not too familiar with generator functions. |
||
const args = node.arguments; | ||
const firstArg = args[0]; | ||
const lastArg = args[args.length - 1]; | ||
const parens = needsParens(node); | ||
const comments = sourceCode.getCommentsInside(node); | ||
const replaceObjectAssignStart = fixer.replaceTextRange( | ||
[node.range[0], firstArg.range[0]], | ||
`${parens ? "({" : "{"}` | ||
); | ||
|
||
const handleArgs = args | ||
.map((arg, i, arr) => formatArg(arg, i + 1 >= arr.length, sourceCode, comments)) | ||
.filter(arg => arg !== "," && arg !== ""); | ||
|
||
const insertBody = fixer.replaceTextRange([firstArg.range[0], lastArg.range[1]], handleArgs.join("")); | ||
const replaceObjectAssignEnd = fixer.replaceTextRange([lastArg.range[1], node.range[1]], `${parens ? "})" : "}"}`); | ||
|
||
return [ | ||
replaceObjectAssignStart, | ||
insertBody, | ||
replaceObjectAssignEnd | ||
]; | ||
}; | ||
} | ||
|
||
/** | ||
* Autofixes the Object.assign call with a single object literal as an argument | ||
* @param {ASTNode|null} node - The node that the rule warns on, i.e. the Object.assign call | ||
* @param {string} sourceCode - sourceCode of the Object.assign call | ||
* @returns {Function} autofixer - replaces the Object.assign with a object literal. | ||
*/ | ||
function autofixObjectLiteral(node, sourceCode) { | ||
return fixer => { | ||
const argument = node.arguments[0]; | ||
const parens = needsParens(node); | ||
|
||
return fixer.replaceText(node, `${parens ? "(" : ""}${sourceCode.text.slice(argument.range[0], argument.range[1])}${parens ? ")" : ""}`); | ||
}; | ||
} | ||
|
||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: | ||
"disallow using Object.assign with an object literal as the first argument and prefer the use of object spread instead.", | ||
category: "Stylistic Issues", | ||
recommended: false, | ||
url: "https://eslint.org/docs/rules/prefer-object-spread" | ||
}, | ||
schema: [], | ||
fixable: "code", | ||
messages: { | ||
useSpreadMessage: "Use an object spread instead of `Object.assign` eg: `{ ...foo }`", | ||
useLiteralMessage: "Use an object literal instead of `Object.assign`. eg: `{ foo: bar }`" | ||
} | ||
}, | ||
|
||
create: function rule(context) { | ||
const sourceCode = context.getSourceCode(); | ||
let overWritingNativeObject = false; | ||
|
||
return { | ||
VariableDeclaration(node) { | ||
|
||
/* | ||
* If a declared variable is overwriting the native `Object`, eg. `const Object = 'something'` | ||
* we want to skip warning on them since the behavior might be different. | ||
*/ | ||
if (node.declarations.length) { | ||
const declarationNames = node.declarations.map(dec => dec.id); | ||
const nativeObjectOverride = declarationNames.filter(identifier => identifier.name === "Object"); | ||
|
||
if (nativeObjectOverride.length) { | ||
overWritingNativeObject = true; | ||
} | ||
} | ||
}, | ||
AssignmentExpression(node) { | ||
|
||
/* | ||
* If an assignment is reassigning the native `Object`, eg. `Object = 'something'` | ||
* we want to skip warning on them since the behavior might be different. | ||
*/ | ||
if (node.left.name === "Object") { | ||
overWritingNativeObject = true; | ||
} | ||
}, | ||
CallExpression(node) { | ||
|
||
/* | ||
* The condition below is cases where Object.assign has a single argument and | ||
* that argument is an object literal. e.g. `Object.assign({ foo: bar })`. | ||
* For now, we will warn on this case and autofix it. | ||
*/ | ||
if ( | ||
!overWritingNativeObject && | ||
node.arguments.length === 1 && | ||
node.arguments[0].type === "ObjectExpression" && | ||
isObjectAssign(node) | ||
) { | ||
context.report({ | ||
node, | ||
messageId: "useLiteralMessage", | ||
fix: autofixObjectLiteral(node, sourceCode) | ||
}); | ||
} | ||
|
||
/* | ||
* The condition below warns on `Object.assign` calls that that have | ||
* an object literal as the first argument and have a second argument | ||
* that can be spread. e.g `Object.assign({ foo: bar }, baz)` | ||
*/ | ||
if ( | ||
!overWritingNativeObject && | ||
node.arguments.length > 1 && | ||
node.arguments[0].type === "ObjectExpression" && | ||
isObjectAssign(node) | ||
) { | ||
context.report({ | ||
node, | ||
messageId: "useSpreadMessage", | ||
fix: autofixSpread(node, sourceCode) | ||
}); | ||
} | ||
} | ||
}; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use
matchAll
here? Since this regex doesn't have any capturing groups, it seems like this could just be done with:That would avoid the need to add a dependency on
string.prototype.matchall
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a global regex,
.match
does not return an array of match objects, so there's no way to get the index (position) of each match - this is the entire reason.matchAll
is needed in the language.So, no, I don't think there's any way to avoid the dependency (without inlining it, of course, which would be a terrible idea)