Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prefer-modern-dom-apis rule #362

Merged
merged 14 commits into from Dec 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 52 additions & 0 deletions docs/rules/prefer-modern-dom-apis.md
@@ -0,0 +1,52 @@
# Prefer modern DOM APIs

Enforces the use of:

- [childNode.replaceWith(newNode)](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/replaceWith) over [parentNode.replaceChild(newNode, oldNode)](https://developer.mozilla.org/en-US/docs/Web/API/Node/replaceChild)
- [referenceNode.before(newNode)](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/before) over [parentNode.insertBefore(newNode, referenceNode)](https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore)
- [referenceNode.before('text')](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/before) over [referenceNode.insertAdjacentText('beforebegin', 'text')](https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentText)
- [referenceNode.before(newNode)](https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/before) over [referenceNode.insertAdjacentElement('beforebegin', newNode)](https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentElement)

There are some advantages of using the newer DOM APIs, like:

- Traversing to the parent node is not necessary.
- Appending multiple nodes at once.
- Both [`DOMString`](https://developer.mozilla.org/en-US/docs/Web/API/DOMString) and [DOM node objects](https://developer.mozilla.org/en-US/docs/Web/API/Element) can be manipulated.

This rule is fixable.

## Fail

```js
foo.replaceChild(baz, bar);

foo.insertBefore(baz, bar);

foo.insertAdjacentText('position', bar);

foo.insertAdjacentElement('position', bar);
```

## Pass

```js
foo.replaceWith(bar);
foo.replaceWith('bar');
foo.replaceWith(bar, 'baz'));

foo.before(bar)
foo.before('bar')
foo.before(bar, 'baz')

foo.prepend(bar)
foo.prepend('bar')
foo.prepend(bar, 'baz')

foo.append(bar)
foo.append('bar')
foo.append(bar, 'baz')

foo.after(bar)
foo.after('bar')
foo.after(bar, 'baz')
```
1 change: 1 addition & 0 deletions index.js
Expand Up @@ -49,6 +49,7 @@ module.exports = {
'unicorn/prefer-exponentiation-operator': 'error',
'unicorn/prefer-flat-map': 'error',
'unicorn/prefer-includes': 'error',
'unicorn/prefer-modern-dom-apis': 'error',
'unicorn/prefer-negative-index': 'error',
'unicorn/prefer-node-append': 'error',
'unicorn/prefer-node-remove': 'error',
Expand Down
4 changes: 3 additions & 1 deletion readme.md
Expand Up @@ -11,7 +11,7 @@ You might want to check out [XO](https://github.com/xojs/xo), which includes thi

mcmxcdev marked this conversation as resolved.
Show resolved Hide resolved
## Install

```
```console
$ npm install --save-dev eslint eslint-plugin-unicorn
```

Expand Down Expand Up @@ -67,6 +67,7 @@ Configure it in `package.json`.
"unicorn/prefer-exponentiation-operator": "error",
"unicorn/prefer-flat-map": "error",
"unicorn/prefer-includes": "error",
"unicorn/prefer-modern-dom-apis": "error",
"unicorn/prefer-negative-index": "error",
"unicorn/prefer-node-append": "error",
"unicorn/prefer-node-remove": "error",
Expand Down Expand Up @@ -120,6 +121,7 @@ Configure it in `package.json`.
- [prefer-exponentiation-operator](docs/rules/prefer-exponentiation-operator.md) - Prefer the exponentiation operator over `Math.pow()` *(fixable)*
- [prefer-flat-map](docs/rules/prefer-flat-map.md) - Prefer `.flatMap(…)` over `.map(…).flat()`. *(fixable)*
- [prefer-includes](docs/rules/prefer-includes.md) - Prefer `.includes()` over `.indexOf()` when checking for existence or non-existence. *(fixable)*
- [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)*
Expand Down
158 changes: 158 additions & 0 deletions rules/prefer-modern-dom-apis.js
@@ -0,0 +1,158 @@
'use strict';
const getDocumentationUrl = require('./utils/get-documentation-url');

const getArgumentNameForReplaceChildOrInsertBefore = nodeArguments => {
if (nodeArguments.type === 'Identifier') {
return nodeArguments.name;
}
};

const forbiddenIdentifierNames = new Map([
['replaceChild', 'replaceWith'],
['insertBefore', 'before']
]);

const isPartOfVariableAssignment = nodeParentType => {
if (nodeParentType === 'VariableDeclarator' || nodeParentType === 'AssignmentExpression') {
return true;
}

return false;
};
Comment on lines +15 to +21
Copy link
Collaborator

@fisker fisker Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is enough,

simple one:

console.log(referenceNode.insertAdjacentElement("beforebegin", newNode));


const checkForReplaceChildOrInsertBefore = (context, node) => {
const identifierName = node.callee.property.name;

// Return early when specified methods don't exist in forbiddenIdentifierNames
if (!forbiddenIdentifierNames.has(identifierName)) {
return;
}

const nodeArguments = node.arguments;
const newChildNodeArgument = getArgumentNameForReplaceChildOrInsertBefore(
nodeArguments[0]
);
const oldChildNodeArgument = getArgumentNameForReplaceChildOrInsertBefore(
nodeArguments[1]
);

// Return early in case that one of the provided arguments is not a node
if (!newChildNodeArgument || !oldChildNodeArgument) {
return;
}

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
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;
}

return context.report({
node,
message: `Prefer \`${oldChildNodeArgument}.${preferredSelector}(${newChildNodeArgument})\` over \`${parentNode}.${identifierName}(${newChildNodeArgument}, ${oldChildNodeArgument})\`.`,
fix
});
};

// Handle both `Identifier` and `Literal` because the preferred selectors support nodes and DOMString.
const getArgumentNameForInsertAdjacentMethods = nodeArguments => {
if (nodeArguments.type === 'Identifier') {
return nodeArguments.name;
}

if (nodeArguments.type === 'Literal') {
return nodeArguments.raw;
}
};

const positionReplacers = new Map([
['beforebegin', 'before'],
['afterbegin', 'prepend'],
['beforeend', 'append'],
['afterend', 'after']
]);

const checkForInsertAdjacentTextOrInsertAdjacentElement = (context, node) => {
const identifierName = node.callee.property.name;

// Return early when method name is not one of the targeted ones.
if (
identifierName !== 'insertAdjacentText' &&
identifierName !== 'insertAdjacentElement'
) {
mcmxcdev marked this conversation as resolved.
Show resolved Hide resolved
return;
}

const nodeArguments = node.arguments;
const positionArgument = getArgumentNameForInsertAdjacentMethods(nodeArguments[0]);
const positionAsValue = nodeArguments[0].value;

// Return early when specified position value of first argument is not a recognized value.
if (!positionReplacers.has(positionAsValue)) {
return;
}

const referenceNode = node.callee.object.name;
const preferredSelector = positionReplacers.get(positionAsValue);
const insertedTextArgument = getArgumentNameForInsertAdjacentMethods(
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;
}

return context.report({
node,
message: `Prefer \`${referenceNode}.${preferredSelector}(${insertedTextArgument})\` over \`${referenceNode}.${identifierName}(${positionArgument}, ${insertedTextArgument})\`.`,
fix
});
};

const create = context => {
return {
CallExpression(node) {
if (
node.callee.type === 'MemberExpression' &&
node.arguments.length === 2
Copy link
Contributor

@MrHen MrHen Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can include these checks in the selector pattern for the rule. Not required for the PR but in case you hadn't seen it yet, here are the docs: https://eslint.org/docs/developer-guide/selectors

) {
checkForReplaceChildOrInsertBefore(context, node);
checkForInsertAdjacentTextOrInsertAdjacentElement(context, node);
}
}
};
};

module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
url: getDocumentationUrl(__filename)
},
fixable: 'code'
}
};