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.

Sign up for GitHub

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

Jump to bottom

prefer-modern-dom-apis: Only fix when expression is not used #503

Merged
merged 3 commits into from Feb 1, 2020
Merged

prefer-modern-dom-apis: Only fix when expression is not used #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff4a6ca
fix(`prefer-modern-dom-apis`): Switch to whitelist for determining wh…
brettz9 Jan 24, 2020
2a1efcd
Refactoring: Invert to `isValueNotUsable`
brettz9 Feb 1, 2020
a5d5d53
Refactor rules/utils/is-value-not-usable.js
brettz9 Feb 1, 2020
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
53 changes: 21 additions & 32 deletions rules/prefer-modern-dom-apis.js
View file
Open in desktop
@@ -1,5 +1,6 @@
'use strict';
const getDocumentationUrl = require('./utils/get-documentation-url');
const isValueNotUsable = require('./utils/is-value-not-usable');

const getArgumentNameForReplaceChildOrInsertBefore = nodeArguments => {
if (nodeArguments.type === 'Identifier') {
Expand All @@ -12,14 +13,6 @@ const forbiddenIdentifierNames = new Map([
['insertBefore', 'before']
]);

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

return false;
};

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

Expand All @@ -42,24 +35,22 @@ const checkForReplaceChildOrInsertBefore = (context, node) => {
}

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
// 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 reported
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;
}
const fix = isValueNotUsable(node) ?
// 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.
fixer => fixer.replaceText(
node,
`${oldChildNodeArgument}.${preferredSelector}(${newChildNodeArgument})`
) :
undefined;

return context.report({
node,
Expand Down Expand Up @@ -112,18 +103,16 @@ const checkForInsertAdjacentTextOrInsertAdjacentElement = (context, node) => {
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;
}
const fix = identifierName === 'insertAdjacentElement' && !isValueNotUsable(node) ?
// Report error when the method is part of a variable assignment
// but don't offer to autofix `.insertAdjacentElement()`
// which doesn't have a return value.
undefined :
fixer =>
fixer.replaceText(
node,
`${referenceNode}.${preferredSelector}(${insertedTextArgument})`
);

return context.report({
node,
Expand Down
4 changes: 2 additions & 2 deletions rules/prefer-node-append.js
View file
Open in desktop
@@ -1,6 +1,6 @@
'use strict';
const getDocumentationUrl = require('./utils/get-documentation-url');
const isValueUsed = require('./utils/is-value-used');
const isValueNotUsable = require('./utils/is-value-not-usable');

const getMethodName = memberExpression => memberExpression.property.name;

Expand All @@ -10,7 +10,7 @@ const create = context => {
const {callee} = node;

if (callee.type === 'MemberExpression' && getMethodName(callee) === 'appendChild') {
const fix = isValueUsed(node) ? undefined : fixer => fixer.replaceText(callee.property, 'append');
const fix = isValueNotUsable(node) ? fixer => fixer.replaceText(callee.property, 'append') : undefined;

context.report({
node,
Expand Down
4 changes: 2 additions & 2 deletions rules/prefer-node-remove.js
View file
Open in desktop
@@ -1,6 +1,6 @@
'use strict';
const getDocumentationUrl = require('./utils/get-documentation-url');
const isValueUsed = require('./utils/is-value-used');
const isValueNotUsable = require('./utils/is-value-not-usable');

const getMethodName = callee => {
const {property} = callee;
Expand Down Expand Up @@ -66,7 +66,7 @@ const create = context => {
const argumentName = getArgumentName(node.arguments);

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

context.report({
node,
Expand Down
3 changes: 3 additions & 0 deletions rules/utils/is-value-not-usable.js
View file
Open in desktop
@@ -0,0 +1,3 @@
'use strict';

module.exports = ({parent}) => !parent || parent.type === 'ExpressionStatement';
25 changes: 0 additions & 25 deletions rules/utils/is-value-used.js
View file
Open in desktop

This file was deleted.

74 changes: 73 additions & 1 deletion test/prefer-modern-dom-apis.js
View file
Open in desktop
Expand Up @@ -102,7 +102,7 @@ ruleTester.run('prefer-modern-dom-apis', rule, {
'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.'
}
],
output: 'beta.before(alfa).insertBefore(charlie, delta);'
output: 'parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta);'
},
{
code: 'const foo = parentNode.insertBefore(alfa, beta);',
Expand All @@ -124,6 +124,28 @@ ruleTester.run('prefer-modern-dom-apis', rule, {
],
output: 'foo = parentNode.insertBefore(alfa, beta);'
},
{
code: 'new Dom(parentNode.insertBefore(alfa, beta))',
errors: [
{
message:
'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.'
}
],
output: 'new Dom(parentNode.insertBefore(alfa, beta))'
},
{
/* eslint-disable no-template-curly-in-string */
code: '`${parentNode.insertBefore(alfa, beta)}`',
errors: [
{
message:
'Prefer `beta.before(alfa)` over `parentNode.insertBefore(alfa, beta)`.'
}
],
output: '`${parentNode.insertBefore(alfa, beta)}`'
/* eslint-enable no-template-curly-in-string */
},
// Tests for .insertAdjacentText()
{
code: 'referenceNode.insertAdjacentText("beforebegin", "text");',
Expand Down Expand Up @@ -275,6 +297,56 @@ ruleTester.run('prefer-modern-dom-apis', rule, {
}
],
output: 'foo = referenceNode.insertAdjacentElement("beforebegin", newNode);'
},
{
code: 'const foo = [referenceNode.insertAdjacentElement("beforebegin", newNode)]',
errors: [
{
message:
'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.'
}
],
output: 'const foo = [referenceNode.insertAdjacentElement("beforebegin", newNode)]'
},
{
code: 'foo(bar = referenceNode.insertAdjacentElement("beforebegin", newNode))',
errors: [
{
message:
'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.'
}
],
output: 'foo(bar = referenceNode.insertAdjacentElement("beforebegin", newNode))'
},
{
code: 'const foo = () => { return referenceNode.insertAdjacentElement("beforebegin", newNode); }',
errors: [
{
message:
'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.'
}
],
output: 'const foo = () => { return referenceNode.insertAdjacentElement("beforebegin", newNode); }'
},
{
code: 'if (referenceNode.insertAdjacentElement("beforebegin", newNode)) {}',
errors: [
{
message:
'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.'
}
],
output: 'if (referenceNode.insertAdjacentElement("beforebegin", newNode)) {}'
},
{
code: 'const foo = { bar: referenceNode.insertAdjacentElement("beforebegin", newNode) }',
errors: [
{
message:
'Prefer `referenceNode.before(newNode)` over `referenceNode.insertAdjacentElement("beforebegin", newNode)`.'
}
],
output: 'const foo = { bar: referenceNode.insertAdjacentElement("beforebegin", newNode) }'
}
]
});