diff --git a/rules/prefer-dom-node-remove.js b/rules/prefer-dom-node-remove.js index 1016c8d049..ed111da530 100644 --- 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 index 81806495e5..d0b67b86d6 100644 --- 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)), });