Skip to content

Move rules settings to ESLint shared config: part 6 - refactor no-manual-cleanup rule #249

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

Merged
merged 10 commits into from
Nov 4, 2020
Merged

Move rules settings to ESLint shared config: part 6 - refactor no-manual-cleanup rule #249

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7db6f18
refactor(no-manual-cleanup): use custom rule creator
Belco90 Nov 3, 2020
dac0e91
refactor: extract detection utils for import module name
Belco90 Nov 3, 2020
fcf1452
refactor(no-manual-cleanup): use detection helpers for imported modules
Belco90 Nov 3, 2020
9e48245
refactor(no-manual-cleanup): small improvements
Belco90 Nov 3, 2020
8641739
test(no-manual-cleanup): include more variants
Belco90 Nov 3, 2020
0dd98b8
feat(no-manual-cleanup): report custom module
Belco90 Nov 3, 2020
9e77544
refactor: rename type for import module node
Belco90 Nov 3, 2020
07c0be2
refactor: use node utils to know node type
Belco90 Nov 4, 2020
ab35358
refactor: remove unused imports
Belco90 Nov 4, 2020
50a3cef
refactor: remove outdated comment line
Belco90 Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions lib/detect-testing-library-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
import { isLiteral } from './node-utils';
import {
getImportModuleName,
isLiteral,
ModuleImportation,
} from './node-utils';

export type TestingLibrarySettings = {
'testing-library/module'?: string;
Expand All @@ -25,14 +29,11 @@ export type EnhancedRuleCreate<
detectionHelpers: Readonly<DetectionHelpers>
) => TRuleListener;

type ModuleImportation =
| TSESTree.ImportDeclaration
| TSESTree.CallExpression
| null;

export type DetectionHelpers = {
getTestingLibraryImportNode: () => ModuleImportation;
getCustomModuleImportNode: () => ModuleImportation;
getTestingLibraryImportNode: () => ModuleImportation | null;
getCustomModuleImportNode: () => ModuleImportation | null;
getTestingLibraryImportName: () => string | undefined;
getCustomModuleImportName: () => string | undefined;
getIsTestingLibraryImported: () => boolean;
getIsValidFilename: () => boolean;
canReportErrors: () => boolean;
Expand All @@ -52,8 +53,8 @@ export function detectTestingLibraryUtils<
context: TestingLibraryContext<TOptions, TMessageIds>,
optionsWithDefault: Readonly<TOptions>
): TSESLint.RuleListener => {
let importedTestingLibraryNode: ModuleImportation = null;
let importedCustomModuleNode: ModuleImportation = null;
let importedTestingLibraryNode: ModuleImportation | null = null;
let importedCustomModuleNode: ModuleImportation | null = null;

// Init options based on shared ESLint settings
const customModule = context.settings['testing-library/module'];
Expand All @@ -69,6 +70,12 @@ export function detectTestingLibraryUtils<
getCustomModuleImportNode() {
return importedCustomModuleNode;
},
getTestingLibraryImportName() {
return getImportModuleName(importedTestingLibraryNode);
},
getCustomModuleImportName() {
return getImportModuleName(importedCustomModuleNode);
},
Comment on lines +69 to +74
Copy link
Member Author

Choose a reason for hiding this comment

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

Two new helpers to get the name of Testing Library or Custom Module imports 💪

/**
* Gets if Testing Library is considered as imported or not.
*
Expand Down
36 changes: 33 additions & 3 deletions lib/node-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export function isNewExpression(
return node && node.type === 'NewExpression';
}

// TODO: remove this one and use ASTUtils one instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

🧐

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh man, I saw your ASTUtils now... ignore my previous comments. It's awesome they export that 🚀

export function isIdentifier(node: TSESTree.Node): node is TSESTree.Identifier {
return node && node.type === AST_NODE_TYPES.Identifier;
}
Expand Down Expand Up @@ -69,8 +70,10 @@ export function isObjectPattern(
return node && node.type === AST_NODE_TYPES.ObjectPattern;
}

export function isProperty(node: TSESTree.Node): node is TSESTree.Property {
return node && node.type === AST_NODE_TYPES.Property;
export function isProperty(
node: TSESTree.Node | null | undefined
): node is TSESTree.Property {
return node?.type === AST_NODE_TYPES.Property;
}

export function isJSXAttribute(
Expand Down Expand Up @@ -126,6 +129,7 @@ export function hasThenProperty(node: TSESTree.Node): boolean {
);
}

// TODO: remove this one and use ASTUtils one instead
export function isAwaitExpression(
node: TSESTree.Node
): node is TSESTree.AwaitExpression {
Expand Down Expand Up @@ -176,7 +180,7 @@ export function getVariableReferences(
): TSESLint.Scope.Reference[] {
return (
(isVariableDeclarator(node) &&
context.getDeclaredVariables(node)[0].references.slice(1)) ||
context.getDeclaredVariables(node)[0]?.references?.slice(1)) ||
[]
);
}
Expand Down Expand Up @@ -220,3 +224,29 @@ export function isRenderVariableDeclarator(

return false;
}

// TODO: extract into types file?
export type ModuleImportation =
| TSESTree.ImportDeclaration
| TSESTree.CallExpression;

export function getImportModuleName(
node: ModuleImportation | undefined | null
): string | undefined {
// import node of shape: import { foo } from 'bar'
if (
node?.type === AST_NODE_TYPES.ImportDeclaration &&
typeof node.source.value === 'string'
) {
return node.source.value;
}

// import node of shape: const { foo } = require('bar')
if (
node?.type === AST_NODE_TYPES.CallExpression &&
isLiteral(node.arguments[0]) &&
typeof node.arguments[0].value === 'string'
) {
return node.arguments[0].value;
}
}
40 changes: 11 additions & 29 deletions lib/rules/no-dom-import.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {
AST_NODE_TYPES,
TSESTree,
} from '@typescript-eslint/experimental-utils';
import { isIdentifier, isLiteral } from '../node-utils';
import { createTestingLibraryRule } from '../create-testing-library-rule';

export const RULE_NAME = 'no-dom-import';
Expand Down Expand Up @@ -40,11 +39,10 @@ export default createTestingLibraryRule<Options, MessageIds>({

create(context, [framework], helpers) {
function report(
node: TSESTree.ImportDeclaration | TSESTree.Identifier,
node: TSESTree.ImportDeclaration | TSESTree.CallExpression,
moduleName: string
) {
if (framework) {
const isRequire = isIdentifier(node) && node.name === 'require';
Copy link
Collaborator

Choose a reason for hiding this comment

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

small doubt here: I guess some of the rules had support for require, but that only applied to a few. Are we dropping the support for require in all of them?
I am not opposed to it, I'm wondering if we should document it somewhere? (not sure if the docs for this rule and others showed it as part of the valid examples)

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup in this case the require examples are in the docs https://github.com/testing-library/eslint-plugin-testing-library/blob/master/docs/rules/no-dom-import.md

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh ups nevermind, I saw the helpers consider require as well, great! 🚀 you're one step ahead

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I should have pointed this with a comment on this PR. But yest, I basically extracted the imported module checks from this rule (which were the best ones) into detection helpers. So now all refactores rules automatically getting the imports from either import or require syntax ✨

const correctModuleName = moduleName.replace('dom', framework);
context.report({
node,
Expand All @@ -53,18 +51,16 @@ export default createTestingLibraryRule<Options, MessageIds>({
module: correctModuleName,
},
fix(fixer) {
if (isRequire) {
const callExpression = node.parent as TSESTree.CallExpression;
const name = callExpression.arguments[0] as TSESTree.Literal;
if (node.type === AST_NODE_TYPES.CallExpression) {
const name = node.arguments[0] as TSESTree.Literal;

// Replace the module name with the raw module name as we can't predict which punctuation the user is going to use
return fixer.replaceText(
name,
name.raw.replace(moduleName, correctModuleName)
);
} else {
const importDeclaration = node as TSESTree.ImportDeclaration;
const name = importDeclaration.source;
const name = node.source;
return fixer.replaceText(
name,
name.raw.replace(moduleName, correctModuleName)
Expand All @@ -82,36 +78,22 @@ export default createTestingLibraryRule<Options, MessageIds>({

return {
'Program:exit'() {
const importName = helpers.getTestingLibraryImportName();
Copy link
Member Author

Choose a reason for hiding this comment

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

Reusing the new helper in this rule already refactored.

const importNode = helpers.getTestingLibraryImportNode();

if (!importNode) {
return;
}

// import node of shape: import { foo } from 'bar'
if (importNode.type === AST_NODE_TYPES.ImportDeclaration) {
const domModuleName = DOM_TESTING_LIBRARY_MODULES.find(
(module) => module === importNode.source.value
);
const domModuleName = DOM_TESTING_LIBRARY_MODULES.find(
(module) => module === importName
);

domModuleName && report(importNode, domModuleName);
if (!domModuleName) {
return;
}

// import node of shape: const { foo } = require('bar')
if (importNode.type === AST_NODE_TYPES.CallExpression) {
const literalNodeDomModuleName = importNode.arguments.find(
(arg) =>
isLiteral(arg) &&
typeof arg.value === 'string' &&
DOM_TESTING_LIBRARY_MODULES.includes(arg.value)
) as TSESTree.Literal;

literalNodeDomModuleName &&
report(
importNode.callee as TSESTree.Identifier,
literalNodeDomModuleName.value as string
);
}
report(importNode, domModuleName);
},
};
},
Expand Down
136 changes: 59 additions & 77 deletions lib/rules/no-manual-cleanup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,27 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { getDocsUrl } from '../utils';
import {
AST_NODE_TYPES,
ASTUtils,
TSESTree,
TSESLint,
} from '@typescript-eslint/experimental-utils';
import {
getVariableReferences,
isImportDefaultSpecifier,
isLiteral,
isIdentifier,
isImportSpecifier,
isMemberExpression,
isObjectPattern,
isProperty,
isMemberExpression,
isImportSpecifier,
ModuleImportation,
} from '../node-utils';
import { createTestingLibraryRule } from '../create-testing-library-rule';

export const RULE_NAME = 'no-manual-cleanup';
export type MessageIds = 'noManualCleanup';
type Options = [];

const CLEANUP_LIBRARY_REGEX = /(@testing-library\/(preact|react|svelte|vue))|@marko\/testing-library/;
const CLEANUP_LIBRARY_REGEXP = /(@testing-library\/(preact|react|svelte|vue))|@marko\/testing-library/;

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
export default createTestingLibraryRule<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
Expand All @@ -34,50 +39,38 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
},
defaultOptions: [],

create(context) {
let defaultImportFromTestingLibrary: TSESTree.ImportDeclaration;
let defaultRequireFromTestingLibrary:
| TSESTree.Identifier
| TSESTree.ArrayPattern;

create(context, _, helpers) {
// can't find the right type?
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function reportImportReferences(references: any[]) {
if (references && references.length > 0) {
references.forEach((reference) => {
const utilsUsage = reference.identifier.parent;
if (
isMemberExpression(utilsUsage) &&
isIdentifier(utilsUsage.property) &&
utilsUsage.property.name === 'cleanup'
) {
context.report({
node: utilsUsage.property,
messageId: 'noManualCleanup',
});
}
});
}
function reportImportReferences(references: TSESLint.Scope.Reference[]) {
references.forEach((reference) => {
const utilsUsage = reference.identifier.parent;
if (
isMemberExpression(utilsUsage) &&
ASTUtils.isIdentifier(utilsUsage.property) &&
utilsUsage.property.name === 'cleanup'
) {
context.report({
node: utilsUsage.property,
messageId: 'noManualCleanup',
});
}
});
}

return {
ImportDeclaration(node) {
const value = node.source.value as string;
const testingLibraryWithCleanup = value.match(CLEANUP_LIBRARY_REGEX);

// Early return if the library doesn't support `cleanup`
if (!testingLibraryWithCleanup) {
return;
}
function reportCandidateModule(moduleNode: ModuleImportation) {
if (moduleNode.type === AST_NODE_TYPES.ImportDeclaration) {
// case: import utils from 'testing-library-module'
if (isImportDefaultSpecifier(moduleNode.specifiers[0])) {
const { references } = context.getDeclaredVariables(moduleNode)[0];

if (isImportDefaultSpecifier(node.specifiers[0])) {
defaultImportFromTestingLibrary = node;
reportImportReferences(references);
}

const cleanupSpecifier = node.specifiers.find(
// case: import { cleanup } from 'testing-library-module'
const cleanupSpecifier = moduleNode.specifiers.find(
(specifier) =>
isImportSpecifier(specifier) &&
specifier.imported &&
specifier.imported.name === 'cleanup'
);

Expand All @@ -87,31 +80,15 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
messageId: 'noManualCleanup',
});
}
},
[`VariableDeclarator > CallExpression > Identifier[name="require"]`](
node: TSESTree.Identifier
) {
const { arguments: args } = node.parent as TSESTree.CallExpression;

const literalNodeCleanupModuleName = args.find(
(args) =>
isLiteral(args) &&
typeof args.value === 'string' &&
args.value.match(CLEANUP_LIBRARY_REGEX)
);

if (!literalNodeCleanupModuleName) {
return;
}

const declaratorNode = node.parent
.parent as TSESTree.VariableDeclarator;
} else {
const declaratorNode = moduleNode.parent as TSESTree.VariableDeclarator;

if (isObjectPattern(declaratorNode.id)) {
// case: const { cleanup } = require('testing-library-module')
const cleanupProperty = declaratorNode.id.properties.find(
(property) =>
isProperty(property) &&
isIdentifier(property.key) &&
ASTUtils.isIdentifier(property.key) &&
property.key.name === 'cleanup'
);

Expand All @@ -122,24 +99,29 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
});
}
} else {
defaultRequireFromTestingLibrary = declaratorNode.id;
}
},
'Program:exit'() {
if (defaultImportFromTestingLibrary) {
const references = context.getDeclaredVariables(
defaultImportFromTestingLibrary
)[0].references;

// case: const utils = require('testing-library-module')
const references = getVariableReferences(context, declaratorNode);
reportImportReferences(references);
}
}
}

if (defaultRequireFromTestingLibrary) {
const references = context
.getDeclaredVariables(defaultRequireFromTestingLibrary.parent)[0]
.references.slice(1);
return {
'Program:exit'() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This rule is so much simpler now: it just needs to focus on checks around the node imports, but doesn't need to handle anything to find them.

const testingLibraryImportName = helpers.getTestingLibraryImportName();
const testingLibraryImportNode = helpers.getTestingLibraryImportNode();
const customModuleImportNode = helpers.getCustomModuleImportNode();

if (
testingLibraryImportName &&
testingLibraryImportNode &&
testingLibraryImportName.match(CLEANUP_LIBRARY_REGEXP)
) {
reportCandidateModule(testingLibraryImportNode);
}

reportImportReferences(references);
if (customModuleImportNode) {
reportCandidateModule(customModuleImportNode);
}
},
};
Expand Down
Loading