diff --git a/conf/eslint-recommended.js b/conf/eslint-recommended.js index 422e01def6d..4b4ed0ae815 100644 --- a/conf/eslint-recommended.js +++ b/conf/eslint-recommended.js @@ -230,6 +230,7 @@ module.exports = { "prefer-const": "off", "prefer-destructuring": "off", "prefer-numeric-literals": "off", + "prefer-object-spread": "off", "prefer-promise-reject-errors": "off", "prefer-reflect": "off", "prefer-rest-params": "off", diff --git a/docs/rules/prefer-object-spread.md b/docs/rules/prefer-object-spread.md new file mode 100644 index 00000000000..0907170e427 --- /dev/null +++ b/docs/rules/prefer-object-spread.md @@ -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. diff --git a/lib/rules/prefer-object-spread.js b/lib/rules/prefer-object-spread.js new file mode 100644 index 00000000000..6a213c690b8 --- /dev/null +++ b/lib/rules/prefer-object-spread.js @@ -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 => { + 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) + }); + } + } + }; + } +}; diff --git a/package.json b/package.json index 9cc8b83da3d..4d350b93fa1 100644 --- a/package.json +++ b/package.json @@ -67,6 +67,7 @@ "regexpp": "^1.1.0", "require-uncached": "^1.0.3", "semver": "^5.5.0", + "string.prototype.matchall": "^2.0.0", "strip-ansi": "^4.0.0", "strip-json-comments": "^2.0.1", "table": "^4.0.3", diff --git a/tests/lib/rules/prefer-object-spread.js b/tests/lib/rules/prefer-object-spread.js new file mode 100644 index 00000000000..51c7e8be3bd --- /dev/null +++ b/tests/lib/rules/prefer-object-spread.js @@ -0,0 +1,588 @@ +/** + * @fileoverview Prefers object spread property over Object.assign + * @author Sharmila Jesupaul + * See LICENSE file in root directory for full license. + */ + +"use strict"; + +const rule = require("../../../lib/rules/prefer-object-spread"); + +const RuleTester = require("../../../lib/testers/rule-tester"); + +const parserOptions = { + ecmaVersion: 2018, + ecmaFeatures: { + experimentalObjectRestSpread: true + } +}; + +const ruleTester = new RuleTester({ parserOptions }); + +ruleTester.run("prefer-object-spread", rule, { + valid: [ + "Object.assign()", + "let a = Object.assign(a, b)", + "Object.assign(a, b)", + "let a = Object.assign(b, { c: 1 })", + "const bar = { ...foo }", + "Object.assign(...foo)", + "Object.assign(foo, { bar: baz })", + "foo({ foo: 'bar' })", + ` + const Object = {}; + Object.assign({}, foo); + `, + ` + Object = {}; + Object.assign({}, foo); + `, + ` + const Object = {}; + Object.assign({ foo: 'bar' }); + `, + ` + Object = {}; + Object.assign({ foo: 'bar' }); + ` + ], + + invalid: [ + { + code: "Object.assign({}, foo)", + output: "({...foo})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + { + code: "Object.assign({}, { foo: 'bar' })", + output: "({foo: 'bar'})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: "Object.assign({}, baz, { foo: 'bar' })", + output: "({...baz, foo: 'bar'})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: "Object.assign({}, { foo: 'bar', baz: 'foo' })", + output: "({foo: 'bar', baz: 'foo'})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: "Object.assign({ foo: 'bar' }, baz)", + output: "({foo: 'bar', ...baz})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // Many args + { + code: "Object.assign({ foo: 'bar' }, cats, dogs, trees, birds)", + output: "({foo: 'bar', ...cats, ...dogs, ...trees, ...birds})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + { + code: + "Object.assign({ foo: 'bar' }, Object.assign({ bar: 'foo' }, baz))", + output: "({foo: 'bar', ...Object.assign({ bar: 'foo' }, baz)})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + }, + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 31 + } + ] + }, + { + code: + "Object.assign({ foo: 'bar' }, Object.assign({ bar: 'foo' }, Object.assign({}, { superNested: 'butwhy' })))", + output: "({foo: 'bar', ...Object.assign({ bar: 'foo' }, Object.assign({}, { superNested: 'butwhy' }))})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + }, + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 31 + }, + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 61 + } + ] + }, + + // Mix spread in argument + { + code: "Object.assign({foo: 'bar', ...bar}, baz)", + output: "({foo: 'bar', ...bar, ...baz})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // Object shorthand + { + code: "Object.assign({}, { foo, bar, baz })", + output: "({foo, bar, baz})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // Objects with computed properties + { + code: "Object.assign({}, { [bar]: 'foo' })", + output: "({[bar]: 'foo'})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // Objects with spread properties + { + code: "Object.assign({ ...bar }, { ...baz })", + output: "({...bar, ...baz})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // Multiline objects + { + code: `Object.assign({ ...bar }, { + // this is a bar + foo: 'bar', + baz: "cats" + })`, + output: `({...bar, + // this is a bar + foo: 'bar', + baz: "cats" +})`, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + { + code: `Object.assign({ + boo: "lol", + // I'm a comment + dog: "cat" + }, { + // this is a bar + foo: 'bar', + baz: "cats" + })`, + output: `({ + boo: "lol", + // I'm a comment + dog: "cat", + + // this is a bar + foo: 'bar', + baz: "cats" +})`, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + // HTML comment + { + code: `const test = Object.assign({ ...bar }, { + weird + })`, + output: `const test = {...bar, + weird +}`, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 14 + } + ] + }, + + { + code: `const test = Object.assign({ ...bar }, { + foo: 'bar', // inline comment + baz: "cats" + })`, + output: `const test = {...bar, + foo: 'bar', // inline comment + baz: "cats" +}`, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 14 + } + ] + }, + + + { + code: `const test = Object.assign({ ...bar }, { + /** + * foo + */ + foo: 'bar', + baz: "cats" + })`, + output: `const test = {...bar, + /** + * foo + */ + foo: 'bar', + baz: "cats" +}`, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 14 + } + ] + }, + + + /* + * This is a special case where Object.assign is called with a single argument + * and that argument is an object expression. In this case we warn and display + * a message to use an object literal instead. + */ + { + code: "Object.assign({})", + output: "({})", + errors: [ + { + messageId: "useLiteralMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: "Object.assign({ foo: bar })", + output: "({ foo: bar })", + errors: [ + { + messageId: "useLiteralMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + + { + code: ` + const foo = 'bar'; + Object.assign({ foo: bar }) + `, + output: ` + const foo = 'bar'; + ({ foo: bar }) + `, + errors: [ + { + messageId: "useLiteralMessage", + type: "CallExpression", + line: 3, + column: 17 + } + ] + }, + + { + code: ` + foo = 'bar'; + Object.assign({ foo: bar }) + `, + output: ` + foo = 'bar'; + ({ foo: bar }) + `, + errors: [ + { + messageId: "useLiteralMessage", + type: "CallExpression", + line: 3, + column: 17 + } + ] + }, + + { + code: "let a = Object.assign({})", + output: "let a = {}", + errors: [ + { + messageId: "useLiteralMessage", + type: "CallExpression", + line: 1, + column: 9 + } + ] + }, + { + code: "let a = Object.assign({}, a)", + output: "let a = {...a}", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 9 + } + ] + }, + { + code: "let a = Object.assign({ a: 1 }, b)", + output: "let a = {a: 1, ...b}", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 9 + } + ] + }, + { + code: "Object.assign( {}, a, b, )", + output: "({...a, ...b})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: "Object.assign({}, a ? b : {}, b => c, a = 2)", + output: "({...(a ? b : {}), ...(b => c), ...(a = 2)})", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 1 + } + ] + }, + { + code: ` + const someVar = 'foo'; + Object.assign({}, a ? b : {}, b => c, a = 2) + `, + output: ` + const someVar = 'foo'; + ({...(a ? b : {}), ...(b => c), ...(a = 2)}) + `, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 3, + column: 17 + } + ] + }, + { + code: ` + someVar = 'foo'; + Object.assign({}, a ? b : {}, b => c, a = 2) + `, + output: ` + someVar = 'foo'; + ({...(a ? b : {}), ...(b => c), ...(a = 2)}) + `, + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 3, + column: 17 + } + ] + }, + + // Cases where you don't need parens around an object literal + { + code: "[1, 2, Object.assign({}, a)]", + output: "[1, 2, {...a}]", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 8 + } + ] + }, + { + code: "const foo = Object.assign({}, a)", + output: "const foo = {...a}", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 13 + } + ] + }, + { + code: "let a = Object.assign({}, ...b)", + output: "let a = {...b}", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 9 + } + ] + }, + { + code: "function foo() { return Object.assign({}, a) }", + output: "function foo() { return {...a} }", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 25 + } + ] + }, + { + code: "foo(Object.assign({}, a));", + output: "foo({...a});", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 5 + } + ] + }, + { + code: "const x = { foo: 'bar', baz: Object.assign({}, a) }", + output: "const x = { foo: 'bar', baz: {...a} }", + errors: [ + { + messageId: "useSpreadMessage", + type: "CallExpression", + line: 1, + column: 30 + } + ] + } + ] +});