refactor(prefer-wait-for): use new custom rule creator (#255)

* refactor: prefer-wait-for with the new settings

* refactor: generalized util method

* refactor: applied feedback from pr

* test: improve coverage

Author: gndelia

docs/rules/prefer-wait-for.md

@@ -17,6 +17,9 @@ Deprecated `wait` async utils are:
 Examples of **incorrect** code for this rule:
 
 ```js
+import { wait, waitForElement, waitForDomChange } from '@testing-library/dom';
+// this also works for const { wait, waitForElement, waitForDomChange } = require ('@testing-library/dom')
+
 const foo = async () => {
   await wait();
   await wait(() => {});
@@ -25,18 +28,41 @@ const foo = async () => {
   await waitForDomChange(mutationObserverOptions);
   await waitForDomChange({ timeout: 100 });
 };
+
+import * as tl from '@testing-library/dom';
+// this also works for const tl = require('@testing-library/dom')
+const foo = async () => {
+  await tl.wait();
+  await tl.wait(() => {});
+  await tl.waitForElement(() => {});
+  await tl.waitForDomChange();
+  await tl.waitForDomChange(mutationObserverOptions);
+  await tl.waitForDomChange({ timeout: 100 });
+};
 ```
 
 Examples of **correct** code for this rule:
 
 ```js
+import { waitFor, waitForElementToBeRemoved } from '@testing-library/dom';
+// this also works for const { waitFor, waitForElementToBeRemoved } = require('@testing-library/dom')
 const foo = async () => {
   // new waitFor method
   await waitFor(() => {});
 
   // previous waitForElementToBeRemoved is not deprecated
   await waitForElementToBeRemoved(() => {});
 };
+
+import * as tl from '@testing-library/dom';
+// this also works for const tl = require('@testing-library/dom')
+const foo = async () => {
+  // new waitFor method
+  await tl.waitFor(() => {});
+
+  // previous waitForElementToBeRemoved is not deprecated
+  await tl.waitForElementToBeRemoved(() => {});
+};
 ```
 
 ## When Not To Use It

lib/detect-testing-library-utils.ts

@@ -12,6 +12,8 @@ import {
   isImportNamespaceSpecifier,
   isImportSpecifier,
   isProperty,
+  isCallExpression,
+  isObjectPattern,
 } from './node-utils';
 import { ABSENCE_MATCHERS, PRESENCE_MATCHERS } from './utils';
 
@@ -55,6 +57,9 @@ export type DetectionHelpers = {
   findImportedUtilSpecifier: (
     specifierName: string
   ) => TSESTree.ImportClause | TSESTree.Identifier | undefined;
+  isNodeComingFromTestingLibrary: (
+    node: TSESTree.MemberExpression | TSESTree.Identifier
+  ) => boolean;
 };
 
 const DEFAULT_FILENAME_PATTERN = '^.*\\.(test|spec)\\.[jt]sx?$';
@@ -229,6 +234,55 @@ export function detectTestingLibraryUtils<
     const canReportErrors: DetectionHelpers['canReportErrors'] = () => {
       return isTestingLibraryImported() && isValidFilename();
     };
+    /**
+     * Takes a MemberExpression or an Identifier and verifies if its name comes from the import in TL
+     * @param node a MemberExpression (in "foo.property" it would be property) or an Identifier (it should be provided from a CallExpression, for example "foo()")
+     */
+    const isNodeComingFromTestingLibrary: DetectionHelpers['isNodeComingFromTestingLibrary'] = (
+      node: TSESTree.MemberExpression | TSESTree.Identifier
+    ) => {
+      const importOrRequire =
+        getCustomModuleImportNode() ?? getTestingLibraryImportNode();
+      if (!importOrRequire) {
+        return false;
+      }
+      if (ASTUtils.isIdentifier(node)) {
+        if (isImportDeclaration(importOrRequire)) {
+          return importOrRequire.specifiers.some(
+            (s) => isImportSpecifier(s) && s.local.name === node.name
+          );
+        } else {
+          return (
+            ASTUtils.isVariableDeclarator(importOrRequire.parent) &&
+            isObjectPattern(importOrRequire.parent.id) &&
+            importOrRequire.parent.id.properties.some(
+              (p) =>
+                isProperty(p) &&
+                ASTUtils.isIdentifier(p.key) &&
+                ASTUtils.isIdentifier(p.value) &&
+                p.value.name === node.name
+            )
+          );
+        }
+      } else {
+        if (!ASTUtils.isIdentifier(node.object)) {
+          return false;
+        }
+        if (isImportDeclaration(importOrRequire)) {
+          return (
+            isImportDeclaration(importOrRequire) &&
+            isImportNamespaceSpecifier(importOrRequire.specifiers[0]) &&
+            node.object.name === importOrRequire.specifiers[0].local.name
+          );
+        }
+        return (
+          isCallExpression(importOrRequire) &&
+          ASTUtils.isVariableDeclarator(importOrRequire.parent) &&
+          ASTUtils.isIdentifier(importOrRequire.parent.id) &&
+          node.object.name === importOrRequire.parent.id.name
+        );
+      }
+    };
 
     const helpers = {
       getTestingLibraryImportNode,
@@ -244,6 +298,7 @@ export function detectTestingLibraryUtils<
       isAbsenceAssert,
       canReportErrors,
       findImportedUtilSpecifier,
+      isNodeComingFromTestingLibrary,
     };
 
     // Instructions for Testing Library detection.

lib/rules/prefer-wait-for.ts

@@ -1,22 +1,25 @@
-import {
-  ESLintUtils,
-  TSESTree,
-  ASTUtils,
-} from '@typescript-eslint/experimental-utils';
-import { getDocsUrl } from '../utils';
+import { TSESTree, ASTUtils } from '@typescript-eslint/experimental-utils';
+import { createTestingLibraryRule } from '../create-testing-library-rule';
 import {
   isImportSpecifier,
   isMemberExpression,
   findClosestCallExpressionNode,
+  isCallExpression,
+  isImportNamespaceSpecifier,
+  isObjectPattern,
+  isProperty,
 } from '../node-utils';
 
 export const RULE_NAME = 'prefer-wait-for';
-export type MessageIds = 'preferWaitForMethod' | 'preferWaitForImport';
+export type MessageIds =
+  | 'preferWaitForMethod'
+  | 'preferWaitForImport'
+  | 'preferWaitForRequire';
 type Options = [];
 
 const DEPRECATED_METHODS = ['wait', 'waitForElement', 'waitForDomChange'];
 
-export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
+export default createTestingLibraryRule<Options, MessageIds>({
   name: RULE_NAME,
   meta: {
     type: 'suggestion',
@@ -29,14 +32,43 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
       preferWaitForMethod:
         '`{{ methodName }}` is deprecated in favour of `waitFor`',
       preferWaitForImport: 'import `waitFor` instead of deprecated async utils',
+      preferWaitForRequire:
+        'require `waitFor` instead of deprecated async utils',
     },
 
     fixable: 'code',
     schema: [],
   },
   defaultOptions: [],
 
-  create(context) {
+  create(context, _, helpers) {
+    let addWaitFor = false;
+
+    const reportRequire = (node: TSESTree.ObjectPattern) => {
+      context.report({
+        node: node,
+        messageId: 'preferWaitForRequire',
+        fix(fixer) {
+          const excludedImports = [...DEPRECATED_METHODS, 'waitFor'];
+
+          const newAllRequired = node.properties
+            .filter(
+              (s) =>
+                isProperty(s) &&
+                ASTUtils.isIdentifier(s.key) &&
+                !excludedImports.includes(s.key.name)
+            )
+            .map(
+              (s) => ((s as TSESTree.Property).key as TSESTree.Identifier).name
+            );
+
+          newAllRequired.push('waitFor');
+
+          return fixer.replaceText(node, `{ ${newAllRequired.join(',')} }`);
+        },
+      });
+    };
+
     const reportImport = (node: TSESTree.ImportDeclaration) => {
       context.report({
         node: node,
@@ -115,46 +147,57 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
     };
 
     return {
-      'ImportDeclaration[source.value=/testing-library/]'(
-        node: TSESTree.ImportDeclaration
-      ) {
-        const deprecatedImportSpecifiers = node.specifiers.filter(
-          (specifier) =>
-            isImportSpecifier(specifier) &&
-            specifier.imported &&
-            DEPRECATED_METHODS.includes(specifier.imported.name)
-        );
-
-        deprecatedImportSpecifiers.forEach((importSpecifier, i) => {
-          if (i === 0) {
-            reportImport(node);
-          }
-
-          context
-            .getDeclaredVariables(importSpecifier)
-            .forEach((variable) =>
-              variable.references.forEach((reference) =>
-                reportWait(reference.identifier)
-              )
-            );
-        });
+      'CallExpression > MemberExpression'(node: TSESTree.MemberExpression) {
+        const isDeprecatedMethod =
+          ASTUtils.isIdentifier(node.property) &&
+          DEPRECATED_METHODS.includes(node.property.name);
+        if (!isDeprecatedMethod) {
+          // the method does not match a deprecated method
+          return;
+        }
+        if (!helpers.isNodeComingFromTestingLibrary(node)) {
+          // the method does not match from the imported elements from TL (even from custom)
+          return;
+        }
+        addWaitFor = true;
+        reportWait(node.property as TSESTree.Identifier); // compiler is not picking up correctly, it should have inferred it is an identifier
       },
-      'ImportDeclaration[source.value=/testing-library/] > ImportNamespaceSpecifier'(
-        node: TSESTree.ImportNamespaceSpecifier
-      ) {
-        context.getDeclaredVariables(node).forEach((variable) =>
-          variable.references.forEach((reference) => {
-            if (
-              isMemberExpression(reference.identifier.parent) &&
-              ASTUtils.isIdentifier(reference.identifier.parent.property) &&
-              DEPRECATED_METHODS.includes(
-                reference.identifier.parent.property.name
-              )
-            ) {
-              reportWait(reference.identifier.parent.property);
-            }
-          })
-        );
+      'CallExpression > Identifier'(node: TSESTree.Identifier) {
+        if (!DEPRECATED_METHODS.includes(node.name)) {
+          return;
+        }
+
+        if (!helpers.isNodeComingFromTestingLibrary(node)) {
+          return;
+        }
+        addWaitFor = true;
+        reportWait(node);
+      },
+      'Program:exit'() {
+        if (!addWaitFor) {
+          return;
+        }
+        // now that all usages of deprecated methods were replaced, remove the extra imports
+        const testingLibraryNode =
+          helpers.getCustomModuleImportNode() ??
+          helpers.getTestingLibraryImportNode();
+        if (isCallExpression(testingLibraryNode)) {
+          const parent = testingLibraryNode.parent as TSESTree.VariableDeclarator;
+          if (!isObjectPattern(parent.id)) {
+            // if there is no destructuring, there is nothing to replace
+            return;
+          }
+          reportRequire(parent.id);
+        } else {
+          if (
+            testingLibraryNode.specifiers.length === 1 &&
+            isImportNamespaceSpecifier(testingLibraryNode.specifiers[0])
+          ) {
+            // if we import everything, there is nothing to replace
+            return;
+          }
+          reportImport(testingLibraryNode);
+        }
       },
     };
   },

tests/create-testing-library-rule.test.ts

@@ -185,6 +185,16 @@ ruleTester.run(RULE_NAME, rule, {
       queryByRole('button')
     `,
     },
+    {
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+      code: `
+        import * as tl from 'test-utils'
+        const obj = { tl }
+        obj.tl.waitFor(() => {})
+      `,
+    },
   ],
   invalid: [
     // Test Cases for Imports & Filename
@@ -516,5 +526,15 @@ ruleTester.run(RULE_NAME, rule, {
     `,
       errors: [{ line: 4, column: 7, messageId: 'queryByError' }],
     },
+    {
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+      code: `
+        import * as tl from 'test-utils'
+        tl.waitFor(() => {})
+      `,
+      errors: [{ line: 3, column: 9, messageId: 'fakeError' }],
+    },
   ],
 });

tests/fake-rule.ts

@@ -73,6 +73,12 @@ export default createTestingLibraryRule<Options, MessageIds>({
     return {
       'CallExpression Identifier': reportCallExpressionIdentifier,
       MemberExpression: reportMemberExpression,
+      'CallExpression > MemberExpression'(node: TSESTree.MemberExpression) {
+        if (!helpers.isNodeComingFromTestingLibrary(node)) {
+          return;
+        }
+        context.report({ node, messageId: 'fakeError' });
+      },
       ImportDeclaration: reportImportDeclaration,
       'Program:exit'() {
         const importNode = helpers.getCustomModuleImportNode();