Skip to content

Commit

Permalink
Expand reporting for prefer-node-remove (#507)
Browse files Browse the repository at this point in the history
Co-authored-by: fisker Cheung <[email protected]>
  • Loading branch information
brettz9 and fisker authored Feb 13, 2020
1 parent 13e55e6 commit b6e6b32
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 204 deletions.
14 changes: 4 additions & 10 deletions docs/rules/prefer-node-remove.md
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
# Prefer `node.remove()` over `parentNode.removeChild(node)` and `parentElement.removeChild(node)`
# Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`

Enforces the use of, for example, `child.remove();` over `child.parentNode.removeChild(child);` and `child.parentElement.removeChild(child);` for DOM nodes. 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).
Enforces the use of, for example, `child.remove();` over `child.parentNode.removeChild(child);`. 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).

This rule is fixable.


## Fail

```js
foo.parentNode.removeChild(foo);
foo.parentElement.removeChild(foo);
this.parentNode.removeChild(this);
this.parentElement.removeChild(this);
foo.parentNode.removeChild(bar);
foo.parentElement.removeChild(bar);
this.parentNode.removeChild(foo);
this.parentElement.removeChild(foo);
parentNode.removeChild(foo);
parentNode.removeChild(this);
```


Expand Down
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ Configure it in `package.json`.
- [prefer-modern-dom-apis](docs/rules/prefer-modern-dom-apis.md) - Prefer `.before()` over `.insertBefore()`, `.replaceWith()` over `.replaceChild()`, prefer one of `.before()`, `.after()`, `.append()` or `.prepend()` over `insertAdjacentText()` and `insertAdjacentElement()`. *(fixable)*
- [prefer-negative-index](docs/rules/prefer-negative-index.md) - Prefer negative index over `.length - index` for `{String,Array,TypedArray}#slice()` and `Array#splice()`. *(fixable)*
- [prefer-node-append](docs/rules/prefer-node-append.md) - Prefer `Node#append()` over `Node#appendChild()`. *(fixable)*
- [prefer-node-remove](docs/rules/prefer-node-remove.md) - Prefer `node.remove()` over `parentNode.removeChild(node)` and `parentElement.removeChild(node)`. *(fixable)*
- [prefer-node-remove](docs/rules/prefer-node-remove.md) - Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`. *(fixable)*
- [prefer-query-selector](docs/rules/prefer-query-selector.md) - Prefer `.querySelector()` over `.getElementById()`, `.querySelectorAll()` over `.getElementsByClassName()` and `.getElementsByTagName()`. *(partly fixable)*
- [prefer-reflect-apply](docs/rules/prefer-reflect-apply.md) - Prefer `Reflect.apply()` over `Function#apply()`. *(fixable)*
- [prefer-replace-all](docs/rules/prefer-replace-all.md) - Prefer `String#replaceAll()` over regex searches with the global flag. *(fixable)*
Expand Down
53 changes: 15 additions & 38 deletions rules/prefer-node-remove.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,59 +8,36 @@ const selector = methodSelector({
length: 1
});

const getCallerName = callee => {
const {object} = callee;

if (object.type === 'Identifier') {
return object.name;
}

if (object.type === 'MemberExpression') {
const {property} = object;

if (property.type === 'Identifier') {
return property.name;
}
}

return null;
};
const message = 'Prefer Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.';

// TODO: support more types of childNode
const getArgumentName = arguments_ => {
const [identifier] = arguments_;

if (identifier.type === 'ThisExpression') {
return 'this';
}

if (identifier.type === 'Identifier') {
if (identifier.type === 'Identifier' && identifier.name !== 'undefined') {
return identifier.name;
}

return null;
};

const create = context => {
return {
[selector](node) {
const {callee} = node;

const callerName = getCallerName(callee);

if (
(callerName === 'parentNode' || callerName === 'parentElement')
) {
const argumentName = getArgumentName(node.arguments);

if (argumentName) {
const fix = isValueNotUsable(node) ? fixer => fixer.replaceText(node, `${argumentName}.remove()`) : undefined;

context.report({
node,
message: `Prefer \`${argumentName}.remove()\` over \`${callerName}.removeChild(${argumentName})\`.`,
fix
});
}
const argumentName = getArgumentName(node.arguments);

if (argumentName) {
const fix = isValueNotUsable(node) ?
fixer => fixer.replaceText(node, `${argumentName}.remove()`) :
undefined;

context.report({
node,
message,
fix
});
}
}
};
Expand Down
206 changes: 51 additions & 155 deletions test/prefer-node-remove.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,192 +5,88 @@ import rule from '../rules/prefer-node-remove';
const ruleTester = avaRuleTester(test, {
env: {
es6: true
},
parserOptions: {
ecmaVersion: 2020
}
});

const invalidTestCase = (code, output, parentName, argumentName) => {
const message = 'Prefer Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`.';

const invalidTestCase = (code, output) => {
return {
code,
output,
errors: [
{
message: `Prefer \`${argumentName}.remove()\` over \`${parentName}.removeChild(${argumentName})\`.`
message
}
]
};
};

const noFixTestCase = code => ({
code,
output: code,
errors: [
{
message
}
]
});

ruleTester.run('prefer-node-remove', rule, {
valid: [
'foo.remove()',
'this.remove()',
'remove()',
'foo.parentNode.removeChild(\'bar\')',
'parentNode.removeChild(undefined)',

// Not `CallExpression`
'new foo.parentNode.removeChild(foo);',
'new parentNode.removeChild(bar);',
// Not `MemberExpression`
'removeChild(foo);',
// `callee.property` is not a `Identifier`
'foo.parentNode[\'removeChild\'](foo);',
'parentNode[\'removeChild\'](bar);',
// Computed
'foo.parentNode[removeChild](foo);',
'parentNode[removeChild](bar);',
// Not `removeChild`
'foo.parentNode.foo(foo);',
'parentNode.foo(bar);',
// More or less argument(s)
'foo.parentNode.removeChild(foo, two);',
'foo.parentNode.removeChild();',
'foo.parentNode.removeChild(...argumentsArray)'
'parentNode.removeChild(bar, extra);',
'parentNode.removeChild();',
'parentNode.removeChild(...argumentsArray)',

// TODO: support cases bellow, maybe more
'parentNode.removeChild(some.node)',
'parentNode.removeChild(get.child())',
'const foo = async () => parentNode.removeChild(await getChild())',
'parentNode.removeChild((() => childNode)())'
],
invalid: [
invalidTestCase(
'foo.parentNode.removeChild(foo)',
'foo.remove()',
'parentNode',
'foo'
),
invalidTestCase(
'this.parentNode.removeChild(this)',
'this.remove()',
'parentNode',
'this'
'parentNode.removeChild(foo)',
'foo.remove()'
),
invalidTestCase(
'parentNode.removeChild(this)',
'this.remove()',
'parentNode',
'this'
),
invalidTestCase(
'foo.parentNode.removeChild(bar)',
'bar.remove()',
'parentNode',
'bar'
),
invalidTestCase(
'this.parentNode.removeChild(foo)',
'foo.remove()',
'parentNode',
'foo'
),
invalidTestCase(
'foo.parentElement.removeChild(foo)',
'foo.remove()',
'parentElement',
'foo'
),
invalidTestCase(
'this.parentElement.removeChild(this)',
'this.remove()',
'parentElement',
'this'
),
invalidTestCase(
'parentElement.removeChild(this)',
'this.remove()',
'parentElement',
'this'
),
invalidTestCase(
'foo.parentElement.removeChild(bar)',
'bar.remove()',
'parentElement',
'bar'
),
invalidTestCase(
'this.parentElement.removeChild(foo)',
'foo.remove()',
'parentElement',
'foo'
),
invalidTestCase(
'if (foo.parentNode.removeChild(foo)) {}',
'if (foo.parentNode.removeChild(foo)) {}',
'parentNode',
'foo'
),
invalidTestCase(
'var removed = foo.parentNode.removeChild(foo);',
'var removed = foo.parentNode.removeChild(foo);',
'parentNode',
'foo'
),
invalidTestCase(
'const foo = node.parentNode.removeChild(child);',
'const foo = node.parentNode.removeChild(child);',
'parentNode',
'child'
),
invalidTestCase(
'console.log(node.parentNode.removeChild(child));',
'console.log(node.parentNode.removeChild(child));',
'parentNode',
'child'
),
invalidTestCase(
'node.parentNode.removeChild(child) || "foo";',
'node.parentNode.removeChild(child) || "foo";',
'parentNode',
'child'
),
invalidTestCase(
'node.parentNode.removeChild(child) + 0;',
'node.parentNode.removeChild(child) + 0;',
'parentNode',
'child'
),
invalidTestCase(
'node.parentNode.removeChild(child) + 0;',
'node.parentNode.removeChild(child) + 0;',
'parentNode',
'child'
),
invalidTestCase(
'+node.parentNode.removeChild(child);',
'+node.parentNode.removeChild(child);',
'parentNode',
'child'
),
invalidTestCase(
'node.parentNode.removeChild(child) ? "foo" : "bar";',
'node.parentNode.removeChild(child) ? "foo" : "bar";',
'parentNode',
'child'
),
invalidTestCase(
'if (node.parentNode.removeChild(child)) {}',
'if (node.parentNode.removeChild(child)) {}',
'parentNode',
'child'
),
invalidTestCase(
'const foo = [node.parentNode.removeChild(child)]',
'const foo = [node.parentNode.removeChild(child)]',
'parentNode',
'child'
),
invalidTestCase(
'const foo = { bar: node.parentNode.removeChild(child) }',
'const foo = { bar: node.parentNode.removeChild(child) }',
'parentNode',
'child'
),
invalidTestCase(
'function foo() { return node.parentNode.removeChild(child); }',
'function foo() { return node.parentNode.removeChild(child); }',
'parentNode',
'child'
),
invalidTestCase(
'const foo = () => { return node.parentNode.removeChild(child); }',
'const foo = () => { return node.parentNode.removeChild(child); }',
'parentNode',
'child'
),
invalidTestCase(
'foo(bar = node.parentNode.removeChild(child))',
'foo(bar = node.parentNode.removeChild(child))',
'parentNode',
'child'
)
'this.remove()'
),
// Value of `parentNode.removeChild` call is used
noFixTestCase('if (parentNode.removeChild(foo)) {}'),
noFixTestCase('var removed = parentNode.removeChild(child);'),
noFixTestCase('const foo = parentNode.removeChild(child);'),
noFixTestCase('console.log(parentNode.removeChild(child));'),
noFixTestCase('parentNode.removeChild(child) || "foo";'),
noFixTestCase('parentNode.removeChild(child) + 0;'),
noFixTestCase('+parentNode.removeChild(child);'),
noFixTestCase('parentNode.removeChild(child) ? "foo" : "bar";'),
noFixTestCase('if (parentNode.removeChild(child)) {}'),
noFixTestCase('const foo = [parentNode.removeChild(child)]'),
noFixTestCase('const foo = { bar: parentNode.removeChild(child) }'),
noFixTestCase('function foo() { return parentNode.removeChild(child); }'),
noFixTestCase('const foo = () => { return parentElement.removeChild(child); }'),
noFixTestCase('foo(bar = parentNode.removeChild(child))')
]
});

0 comments on commit b6e6b32

Please sign in to comment.