fix(prefer-modern-dom-apis): Ignore other cases where node return v…

…alue is used - Where parent is an `ArrayExpression`, `IfStatement`, `'MemberExpression`, `Property`, `ReturnStatement` Fixes a test which made a fix which would break because of an inability of the replacement API to return a value. Also provides minor optimization to avoid declaring fixer with value when not needed. Also applies (and for `prefer-node-*` rules, switches from) checking `ExpressionStatement` in favor of specific parents (BinaryExpression, CallExpression, ConditionalExpression, LogicalExpression, UnaryExpression), since in theory, the last part of `parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta);` could be checked if the rule were changed to allow such parent expressions rather than requiring a parent member to be a name only (i.e., fixing `parentNode` in `parent.insertBefore()` but ignoring `some.expression().insertBefore()`) since technically such a replacement could be made.
sindresorhus · Jan 24, 2020 · 6c8dab0 · 6c8dab0
1 parent fd46adc
commit 6c8dab0
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 42 deletions.
diff --git 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 isValueUsed = require('./utils/is-value-used');
 
 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 = isValueUsed(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.
+		undefined :
+		fixer => fixer.replaceText(
+			node,
+			`${oldChildNodeArgument}.${preferredSelector}(${newChildNodeArgument})`
+		);
 
 	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' && isValueUsed(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/utils/is-value-used.js b/rules/utils/is-value-used.js
@@ -3,23 +3,20 @@
 const ignoredParentTypes = [
 	'ArrayExpression',
 	'AssignmentExpression',
+	'BinaryExpression',
+	'CallExpression',
+	'ConditionalExpression',
 	'IfStatement',
+	'LogicalExpression',
 	'MemberExpression',
 	'Property',
 	'ReturnStatement',
+	'UnaryExpression',
 	'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));
+	return parent && ignoredParentTypes.includes(parent.type);
 };
diff --git 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);',
@@ -275,6 +275,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) }'
 		}
 	]
 });