Skip to content

Commit b6e9499

Browse files
committed
fix(linter): Fix the logic for unicorn/prefer-dom-node-remove to handle literal callees as well as arguments. (#20059)
Two main changes: - Fix the diagnostic. It was clearly the victim of a copy-paste mistake at some point. Also added a note with a relevant MDN URL. - Add a bunch of tests from the upstream that didn't get ported by our rulegen tooling. And then fix the rule so it doesn't miss out on violations in a lot of the test cases. ```js // Prior to this PR, the rule handled these cases: foo.removeChild(true) foo.removeChild("") foo.removeChild(undefined) // But not these: (true).removeChild(foo) ("").removeChild(foo) (undefined).removeChild(foo) ``` See https://github.com/sindresorhus/eslint-plugin-unicorn/blob/609d4870f3731d39bd5b5f184628e2cf06578dba/test/prefer-dom-node-remove.js for the original test generation logic. AI Disclosure: I used Claude Code to fix up the logic for catching the extra cases.
1 parent 7538f09 commit b6e9499

File tree

2 files changed

+157
-54
lines changed

2 files changed

+157
-54
lines changed

crates/oxc_linter/src/rules/unicorn/prefer_dom_node_remove.rs

Lines changed: 75 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@ use crate::{
1212

1313
fn prefer_dom_node_remove_diagnostic(span: Span) -> OxcDiagnostic {
1414
OxcDiagnostic::warn("Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.")
15-
.with_help("Replace `parentNode.removeChild(childNode)` with `childNode{dotOrQuestionDot}remove()`.")
15+
.with_help("Replace `parentNode.removeChild(childNode)` with `childNode.remove()`.")
1616
.with_label(span)
17+
.with_note("https://developer.mozilla.org/en-US/docs/Web/API/Element/remove")
1718
}
1819

1920
#[derive(Debug, Default, Clone)]
@@ -26,7 +27,8 @@ declare_oxc_lint!(
2627
///
2728
/// ### Why is this bad?
2829
///
29-
/// The DOM function [`Node#remove()`](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove) is preferred over the indirect removal of an object with [`Node#removeChild()`](https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild).
30+
/// The DOM function [`Node#remove()`](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove) is preferred
31+
/// over the indirect removal of an object with [`Node#removeChild()`](https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild).
3032
///
3133
/// ### Examples
3234
///
@@ -45,6 +47,21 @@ declare_oxc_lint!(
4547
pending
4648
);
4749

50+
/// Returns `true` if the expression is a type that can never be a DOM node
51+
/// (literals, null, undefined, arrays, arrow functions, classes, functions, objects, template literals).
52+
fn is_non_dom_node(expr: &Expression) -> bool {
53+
matches!(
54+
expr,
55+
Expression::ArrayExpression(_)
56+
| Expression::ArrowFunctionExpression(_)
57+
| Expression::ClassExpression(_)
58+
| Expression::FunctionExpression(_)
59+
| Expression::ObjectExpression(_)
60+
| Expression::TemplateLiteral(_)
61+
) || expr.is_literal()
62+
|| expr.is_null_or_undefined()
63+
}
64+
4865
impl Rule for PreferDomNodeRemove {
4966
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
5067
let AstKind::CallExpression(call_expr) = node.kind() else {
@@ -59,22 +76,20 @@ impl Rule for PreferDomNodeRemove {
5976
return;
6077
}
6178

79+
// Check if the callee object (the thing `.removeChild()` is called on) is a non-DOM type
80+
if let Some(member_expr) = call_expr.callee.get_member_expr() {
81+
let object = member_expr.object().without_parentheses();
82+
if is_non_dom_node(object) {
83+
return;
84+
}
85+
}
86+
6287
let Some(expr) = call_expr.arguments[0].as_expression() else {
6388
return;
6489
};
6590

6691
let expr = expr.without_parentheses();
67-
if matches!(
68-
expr,
69-
Expression::ArrayExpression(_)
70-
| Expression::ArrowFunctionExpression(_)
71-
| Expression::ClassExpression(_)
72-
| Expression::FunctionExpression(_)
73-
| Expression::ObjectExpression(_)
74-
| Expression::TemplateLiteral(_)
75-
) || expr.is_literal()
76-
|| expr.is_null_or_undefined()
77-
{
92+
if is_non_dom_node(expr) {
7893
return;
7994
}
8095

@@ -96,14 +111,61 @@ fn test() {
96111
"parentNode.removeChild(undefined)",
97112
"new parentNode.removeChild(bar);",
98113
"removeChild(foo);",
114+
// `callee.property` is not an `Identifier`
99115
// TODO: Get this passing.
100116
// "parentNode['removeChild'](bar);",
117+
// Computed
101118
"parentNode[removeChild](bar);",
102119
"parentNode.foo(bar);",
120+
// More or less argument(s)
103121
"parentNode.removeChild(bar, extra);",
104122
"parentNode.removeChild();",
105123
"parentNode.removeChild(...argumentsArray)",
124+
// Optional call
106125
"parentNode.removeChild?.(foo)",
126+
// The following are all generated from this JS code upstream:
127+
// ...notDomNodeTypes.map(data => `(${data}).removeChild(foo)`),
128+
// ...notDomNodeTypes.map(data => `foo.removeChild(${data})`),
129+
"([]).removeChild(foo)",
130+
"([element]).removeChild(foo)",
131+
"([...elements]).removeChild(foo)",
132+
"(() => {}).removeChild(foo)",
133+
"(class Node {}).removeChild(foo)",
134+
"(function() {}).removeChild(foo)",
135+
"(0).removeChild(foo)",
136+
"(1).removeChild(foo)",
137+
"(0.1).removeChild(foo)",
138+
r#"("").removeChild(foo)"#,
139+
r#"("string").removeChild(foo)"#,
140+
"(/regex/).removeChild(foo)",
141+
"(null).removeChild(foo)",
142+
"(0n).removeChild(foo)",
143+
"(1n).removeChild(foo)",
144+
"(true).removeChild(foo)",
145+
"(false).removeChild(foo)",
146+
"({}).removeChild(foo)",
147+
"(`templateLiteral`).removeChild(foo)",
148+
"(undefined).removeChild(foo)",
149+
"foo.removeChild([])",
150+
"foo.removeChild([element])",
151+
"foo.removeChild([...elements])",
152+
"foo.removeChild(() => {})",
153+
"foo.removeChild(class Node {})",
154+
"foo.removeChild(function() {})",
155+
"foo.removeChild(0)",
156+
"foo.removeChild(1)",
157+
"foo.removeChild(0.1)",
158+
r#"foo.removeChild("")"#,
159+
r#"foo.removeChild("string")"#,
160+
"foo.removeChild(/regex/)",
161+
"foo.removeChild(null)",
162+
"foo.removeChild(0n)",
163+
"foo.removeChild(1n)",
164+
"foo.removeChild(true)",
165+
"foo.removeChild(false)",
166+
"foo.removeChild({})",
167+
"foo.removeChild(`templateLiteral`)",
168+
"foo.removeChild(undefined)",
107169
];
108170

109171
let fail = vec![

0 commit comments

Comments
 (0)