prefer-dom-node-remove: Fix incorrect auto-fix (#2084)

sindresorhus · May 10, 2023 · 74bb36d · 74bb36d
1 parent 4ec6bdf
commit 74bb36d
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 14 deletions.
diff --git a/rules/prefer-dom-node-remove.js b/rules/prefer-dom-node-remove.js
@@ -10,7 +10,7 @@ const ERROR_MESSAGE_ID = 'error';
 const SUGGESTION_MESSAGE_ID = 'suggestion';
 const messages = {
 	[ERROR_MESSAGE_ID]: 'Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.',
-	[SUGGESTION_MESSAGE_ID]: 'Replace `parentNode.removeChild(childNode)` with `childNode.remove()`.',
+	[SUGGESTION_MESSAGE_ID]: 'Replace `parentNode.removeChild(childNode)` with `childNode{{dotOrQuestionDot}}remove()`.',
 };
 
 const selector = [
@@ -23,6 +23,15 @@ const selector = [
 	notDomNodeSelector('arguments.0'),
 ].join('');
 
+// TODO: Don't check node.type twice
+const isMemberExpressionOptionalObject = node =>
+	node.parent.type === 'MemberExpression'
+	&& node.parent.object === node
+	&& (
+		node.parent.optional
+		|| (node.type === 'MemberExpression' && isMemberExpressionOptionalObject(node.object))
+	);
+
 /** @param {import('eslint').Rule.RuleContext} context */
 const create = context => {
 	const {sourceCode} = context;
@@ -37,7 +46,9 @@ const create = context => {
 				messageId: ERROR_MESSAGE_ID,
 			};
 
-			const fix = fixer => {
+			const isOptionalParentNode = isMemberExpressionOptionalObject(parentNode);
+
+			const createFix = (optional = false) => fixer => {
 				let childNodeText = getParenthesizedText(childNode, sourceCode);
 				if (
 					!isParenthesized(childNode, sourceCode)
@@ -50,20 +61,42 @@ const create = context => {
 					childNodeText = `;${childNodeText}`;
 				}
 
-				return fixer.replaceText(node, `${childNodeText}.remove()`);
+				return fixer.replaceText(node, `${childNodeText}${optional ? '?' : ''}.remove()`);
 			};
 
-			if (!hasSideEffect(parentNode, sourceCode) && isValueNotUsable(node) && !node.callee.optional) {
-				problem.fix = fix;
-			} else {
-				problem.suggest = [
-					{
-						messageId: SUGGESTION_MESSAGE_ID,
-						fix,
-					},
-				];
+			if (!hasSideEffect(parentNode, sourceCode) && isValueNotUsable(node)) {
+				if (!isOptionalParentNode) {
+					problem.fix = createFix(false);
+					return problem;
+				}
+
+				// The most common case `foo?.parentNode.remove(foo)`
+				// TODO: Allow case like `foo.bar?.parentNode.remove(foo.bar)`
+				if (
+					node.callee.type === 'MemberExpression'
+					&& !node.callee.optional
+					&& parentNode.type === 'MemberExpression'
+					&& parentNode.optional
+					&& !parentNode.computed
+					&& parentNode.property.type === 'Identifier'
+					&& parentNode.property.name === 'parentNode'
+					&& parentNode.object.type === 'Identifier'
+					&& childNode.type === 'Identifier'
+					&& parentNode.object.name === childNode.name
+				) {
+					problem.fix = createFix(true);
+					return problem;
+				}
 			}
 
+			problem.suggest = (
+				isOptionalParentNode ? [true, false] : [false]
+			).map(optional => ({
+				messageId: SUGGESTION_MESSAGE_ID,
+				data: {dotOrQuestionDot: optional ? '?.' : '.'},
+				fix: createFix(optional),
+			}));
+
 			return problem;
 		},
 	};

diff --git a/test/prefer-dom-node-remove.mjs b/test/prefer-dom-node-remove.mjs
@@ -7,7 +7,7 @@ const {test} = getTester(import.meta);
 const ERROR_MESSAGE_ID = 'error';
 const SUGGESTION_MESSAGE_ID = 'suggestion';
 
-const invalidTestCase = ({code, output, suggestionOutput}) => {
+const invalidTestCase = ({code, output, suggestionOutput, suggestionOutputs}) => {
 	if (suggestionOutput) {
 		return {
 			code,
@@ -17,6 +17,7 @@ const invalidTestCase = ({code, output, suggestionOutput}) => {
 					suggestions: [
 						{
 							messageId: SUGGESTION_MESSAGE_ID,
+							data: {dotOrQuestionDot: '.'},
 							output: suggestionOutput,
 						},
 					],
@@ -25,6 +26,29 @@ const invalidTestCase = ({code, output, suggestionOutput}) => {
 		};
 	}
 
+	if (suggestionOutputs) {
+		return {
+			code,
+			errors: [
+				{
+					messageId: ERROR_MESSAGE_ID,
+					suggestions: [
+						{
+							messageId: SUGGESTION_MESSAGE_ID,
+							data: {dotOrQuestionDot: '?.'},
+							output: suggestionOutputs[0],
+						},
+						{
+							messageId: SUGGESTION_MESSAGE_ID,
+							data: {dotOrQuestionDot: '.'},
+							output: suggestionOutputs[1],
+						},
+					],
+				},
+			],
+		};
+	}
+
 	return {
 		code,
 		output,
@@ -250,7 +274,36 @@ test({
 		// Optional parent
 		{
 			code: 'parentNode?.removeChild(foo)',
-			suggestionOutput: 'foo.remove()',
+			suggestionOutputs: ['foo?.remove()', 'foo.remove()'],
+		},
+		{
+			code: 'foo?.parentNode.removeChild(foo)',
+			output: 'foo?.remove()',
+		},
+		{
+			code: 'foo.parentNode?.removeChild(foo)',
+			suggestionOutputs: ['foo?.remove()', 'foo.remove()'],
+		},
+		{
+			code: 'foo?.parentNode?.removeChild(foo)',
+			suggestionOutputs: ['foo?.remove()', 'foo.remove()'],
+		},
+		{
+			code: 'foo.bar?.parentNode.removeChild(foo.bar)',
+			suggestionOutputs: ['foo.bar?.remove()', 'foo.bar.remove()'],
+		},
+		{
+			code: 'a.b?.c.parentNode.removeChild(foo)',
+			suggestionOutputs: ['foo?.remove()', 'foo.remove()'],
+		},
+		{
+			code: 'a[b?.c].parentNode.removeChild(foo)',
+			output: 'foo.remove()',
+		},
+		// The suggestions are bad, since they will break code
+		{
+			code: 'a?.b.parentNode.removeChild(a.b)',
+			suggestionOutputs: ['a.b?.remove()', 'a.b.remove()'],
 		},
 	].map(options => invalidTestCase(options)),
 });