From 096fead3d99eb9ba3aa2f30e05a95926355c77f6 Mon Sep 17 00:00:00 2001 From: Brett Zamir Date: Sat, 1 Feb 2020 18:16:46 +0800 Subject: [PATCH] prefer-modern-dom-apis: Only fix when expression is not used (#503) --- rules/prefer-modern-dom-apis.js | 53 +++++++++------------ rules/prefer-node-append.js | 4 +- rules/prefer-node-remove.js | 4 +- rules/utils/is-value-not-usable.js | 3 ++ rules/utils/is-value-used.js | 25 ---------- test/prefer-modern-dom-apis.js | 74 +++++++++++++++++++++++++++++- 6 files changed, 101 insertions(+), 62 deletions(-) create mode 100644 rules/utils/is-value-not-usable.js delete mode 100644 rules/utils/is-value-used.js diff --git a/rules/prefer-modern-dom-apis.js b/rules/prefer-modern-dom-apis.js index d8f4214881..fb47042889 100644 --- a/rules/prefer-modern-dom-apis.js +++ b/rules/prefer-modern-dom-apis.js @@ -1,5 +1,6 @@ 'use strict'; const getDocumentationUrl = require('./utils/get-documentation-url'); +const isValueNotUsable = require('./utils/is-value-not-usable'); const getArgumentNameForReplaceChildOrInsertBefore = nodeArguments => { if (nodeArguments.type === 'Identifier') { @@ -12,14 +13,6 @@ const forbiddenIdentifierNames = new Map([ ['insertBefore', 'before'] ]); -const isPartOfVariableAssignment = nodeParentType => { - if (nodeParentType === 'VariableDeclarator' || nodeParentType === 'AssignmentExpression') { - return true; - } - - return false; -}; - const checkForReplaceChildOrInsertBefore = (context, node) => { const identifierName = node.callee.property.name; @@ -42,24 +35,22 @@ const checkForReplaceChildOrInsertBefore = (context, node) => { } const parentNode = node.callee.object.name; - // This check makes sure that only the first method of chained methods with same identifier name e.g: parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta); gets transformed + // This check makes sure that only the first method of chained methods with same identifier name e.g: parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta); gets reported if (!parentNode) { return; } const preferredSelector = forbiddenIdentifierNames.get(identifierName); - let fix = fixer => fixer.replaceText( - node, - `${oldChildNodeArgument}.${preferredSelector}(${newChildNodeArgument})` - ); - - // Report error when the method is part of a variable assignment - // but don't offer to autofix `.replaceWith()` and `.before()` - // which don't have a return value. - if (isPartOfVariableAssignment(node.parent.type)) { - fix = undefined; - } + const fix = isValueNotUsable(node) ? + // Report error when the method is part of a variable assignment + // but don't offer to autofix `.replaceWith()` and `.before()` + // which don't have a return value. + fixer => fixer.replaceText( + node, + `${oldChildNodeArgument}.${preferredSelector}(${newChildNodeArgument})` + ) : + undefined; return context.report({ node, @@ -112,18 +103,16 @@ const checkForInsertAdjacentTextOrInsertAdjacentElement = (context, node) => { nodeArguments[1] ); - let fix = fixer => - fixer.replaceText( - node, - `${referenceNode}.${preferredSelector}(${insertedTextArgument})` - ); - - // Report error when the method is part of a variable assignment - // but don't offer to autofix `.insertAdjacentElement()` - // which don't have a return value. - if (identifierName === 'insertAdjacentElement' && isPartOfVariableAssignment(node.parent.type)) { - fix = undefined; - } + const fix = identifierName === 'insertAdjacentElement' && !isValueNotUsable(node) ? + // Report error when the method is part of a variable assignment + // but don't offer to autofix `.insertAdjacentElement()` + // which doesn't have a return value. + undefined : + fixer => + fixer.replaceText( + node, + `${referenceNode}.${preferredSelector}(${insertedTextArgument})` + ); return context.report({ node, diff --git a/rules/prefer-node-append.js b/rules/prefer-node-append.js index 4b69928c1d..ebe51277b6 100644 --- a/rules/prefer-node-append.js +++ b/rules/prefer-node-append.js @@ -1,6 +1,6 @@ 'use strict'; const getDocumentationUrl = require('./utils/get-documentation-url'); -const isValueUsed = require('./utils/is-value-used'); +const isValueNotUsable = require('./utils/is-value-not-usable'); const getMethodName = memberExpression => memberExpression.property.name; @@ -10,7 +10,7 @@ const create = context => { const {callee} = node; if (callee.type === 'MemberExpression' && getMethodName(callee) === 'appendChild') { - const fix = isValueUsed(node) ? undefined : fixer => fixer.replaceText(callee.property, 'append'); + const fix = isValueNotUsable(node) ? fixer => fixer.replaceText(callee.property, 'append') : undefined; context.report({ node, diff --git a/rules/prefer-node-remove.js b/rules/prefer-node-remove.js index b5d6a28079..2028e99f6d 100644 --- a/rules/prefer-node-remove.js +++ b/rules/prefer-node-remove.js @@ -1,6 +1,6 @@ 'use strict'; const getDocumentationUrl = require('./utils/get-documentation-url'); -const isValueUsed = require('./utils/is-value-used'); +const isValueNotUsable = require('./utils/is-value-not-usable'); const getMethodName = callee => { const {property} = callee; @@ -66,7 +66,7 @@ const create = context => { const argumentName = getArgumentName(node.arguments); if (argumentName) { - const fix = isValueUsed(node) ? undefined : fixer => fixer.replaceText(node, `${argumentName}.remove()`); + const fix = isValueNotUsable(node) ? fixer => fixer.replaceText(node, `${argumentName}.remove()`) : undefined; context.report({ node, diff --git a/rules/utils/is-value-not-usable.js b/rules/utils/is-value-not-usable.js new file mode 100644 index 0000000000..fe03b215cc --- /dev/null +++ b/rules/utils/is-value-not-usable.js @@ -0,0 +1,3 @@ +'use strict'; + +module.exports = ({parent}) => !parent || parent.type === 'ExpressionStatement'; diff --git a/rules/utils/is-value-used.js b/rules/utils/is-value-used.js deleted file mode 100644 index f468361a48..0000000000 --- a/rules/utils/is-value-used.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; - -const ignoredParentTypes = [ - 'ArrayExpression', - 'AssignmentExpression', - 'IfStatement', - 'MemberExpression', - 'Property', - 'ReturnStatement', - 'VariableDeclarator' -]; - -const ignoredGrandparentTypes = [ - 'ExpressionStatement' -]; - -module.exports = function (node) { - const {parent} = node; - const { - parent: grandparent - } = (parent || {}); - - return (parent && ignoredParentTypes.includes(parent.type)) || - (grandparent && ignoredGrandparentTypes.includes(grandparent.type)); -}; diff --git a/test/prefer-modern-dom-apis.js b/test/prefer-modern-dom-apis.js index 0ee6595cea..5c5be5b4a7 100644 --- a/test/prefer-modern-dom-apis.js +++ b/test/prefer-modern-dom-apis.js @@ -102,7 +102,7 @@ ruleTester.run('prefer-modern-dom-apis', rule, { 'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.' } ], - output: 'beta.before(alfa).insertBefore(charlie, delta);' + output: 'parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta);' }, { code: 'const foo = parentNode.insertBefore(alfa, beta);', @@ -124,6 +124,28 @@ ruleTester.run('prefer-modern-dom-apis', rule, { ], output: 'foo = parentNode.insertBefore(alfa, beta);' }, + { + code: 'new Dom(parentNode.insertBefore(alfa, beta))', + errors: [ + { + message: + 'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.' + } + ], + output: 'new Dom(parentNode.insertBefore(alfa, beta))' + }, + { + /* eslint-disable no-template-curly-in-string */ + code: '`${parentNode.insertBefore(alfa, beta)}`', + errors: [ + { + message: + 'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.' + } + ], + output: '`${parentNode.insertBefore(alfa, beta)}`' + /* eslint-enable no-template-curly-in-string */ + }, // Tests for .insertAdjacentText() { code: 'referenceNode.insertAdjacentText("beforebegin", "text");', @@ -275,6 +297,56 @@ ruleTester.run('prefer-modern-dom-apis', rule, { } ], output: 'foo = referenceNode.insertAdjacentElement("beforebegin", newNode);' + }, + { + code: 'const foo = [referenceNode.insertAdjacentElement("beforebegin", newNode)]', + errors: [ + { + message: + 'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.' + } + ], + output: 'const foo = [referenceNode.insertAdjacentElement("beforebegin", newNode)]' + }, + { + code: 'foo(bar = referenceNode.insertAdjacentElement("beforebegin", newNode))', + errors: [ + { + message: + 'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.' + } + ], + output: 'foo(bar = referenceNode.insertAdjacentElement("beforebegin", newNode))' + }, + { + code: 'const foo = () => { return referenceNode.insertAdjacentElement("beforebegin", newNode); }', + errors: [ + { + message: + 'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.' + } + ], + output: 'const foo = () => { return referenceNode.insertAdjacentElement("beforebegin", newNode); }' + }, + { + code: 'if (referenceNode.insertAdjacentElement("beforebegin", newNode)) {}', + errors: [ + { + message: + 'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.' + } + ], + output: 'if (referenceNode.insertAdjacentElement("beforebegin", newNode)) {}' + }, + { + code: 'const foo = { bar: referenceNode.insertAdjacentElement("beforebegin", newNode) }', + errors: [ + { + message: + 'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.' + } + ], + output: 'const foo = { bar: referenceNode.insertAdjacentElement("beforebegin", newNode) }' } ] });