feat: add internal eslint plugin for repo-specific lint rules (#1373)

There's a few things I want to enforce internally.
Save us from having to communicate these things in PR reviews.

ensuring people don't do import ts from 'typescript' in packages.
this breaks compat with users that don't use allowSyntheticDefaultImports
ensuring people don't accidentally do import {} from '@typescript-eslint/typescript-estree' from the plugins, where the package isn't a dependency.
this breaks encapsulation, and will cause problems if we move things around in future
Adding this in now reduces the barrier to entry, meaning we can easily add rules to warn against patterns we see people do in the future.

.eslintrc.js

@@ -6,6 +6,7 @@ module.exports = {
     'jest',
     'import',
     'eslint-comments',
+    '@typescript-eslint/internal',
   ],
   env: {
     es6: true,
@@ -117,6 +118,11 @@ module.exports = {
     'import/no-self-import': 'error',
     // Require modules with a single export to use a default export
     'import/prefer-default-export': 'off', // we want everything to be named
+
+    //
+    // Internal repo rules
+    //
+    '@typescript-eslint/internal/no-typescript-default-import': 'error',
   },
   parserOptions: {
     sourceType: 'module',
@@ -127,8 +133,10 @@ module.exports = {
     tsconfigRootDir: __dirname,
   },
   overrides: [
+    // all test files
     {
       files: [
+        'packages/eslint-plugin-internal/tests/**/*.test.ts',
         'packages/eslint-plugin-tslint/tests/**/*.ts',
         'packages/eslint-plugin/tests/**/*.test.ts',
         'packages/parser/tests/**/*.ts',
@@ -138,6 +146,7 @@ module.exports = {
         'jest/globals': true,
       },
       rules: {
+        'eslint-plugin/no-identical-tests': 'error',
         'jest/no-disabled-tests': 'warn',
         'jest/no-focused-tests': 'error',
         'jest/no-alias-methods': 'error',
@@ -152,26 +161,31 @@ module.exports = {
         'jest/valid-expect': 'error',
       },
     },
+    // plugin source files
     {
       files: [
-        'packages/eslint-plugin/tests/**/*.test.ts',
-        'packages/eslint-plugin-tslint/tests/**/*.spec.ts',
+        'packages/eslint-plugin-internal/**/*.ts',
+        'packages/eslint-plugin-tslint/**/*.ts',
+        'packages/eslint-plugin/**/*.ts',
       ],
       rules: {
-        'eslint-plugin/no-identical-tests': 'error',
+        '@typescript-eslint/internal/no-typescript-estree-import': 'error',
       },
     },
+    // rule source files
     {
       files: [
-        'packages/eslint-plugin/src/rules/**/*.ts',
-        'packages/eslint-plugin/src/configs/**/*.ts',
+        'packages/eslint-plugin-internal/src/rules/**/*.ts',
         'packages/eslint-plugin-tslint/src/rules/**/*.ts',
+        'packages/eslint-plugin/src/configs/**/*.ts',
+        'packages/eslint-plugin/src/rules/**/*.ts',
       ],
       rules: {
         // specifically for rules - default exports makes the tooling easier
         'import/no-default-export': 'off',
       },
     },
+    // tools and tests
     {
       files: ['**/tools/**/*.ts', '**/tests/**/*.ts'],
       rules: {

.vscode/launch.json

@@ -20,6 +20,22 @@
           "console": "integratedTerminal",
           "internalConsoleOptions": "neverOpen"
       },
+      {
+          "type": "node",
+          "request": "launch",
+          "name": "Jest Test Current eslint-plugin-internal Rule",
+          "cwd": "${workspaceFolder}/packages/eslint-plugin-internal/",
+          "program": "${workspaceFolder}/node_modules/jest/bin/jest.js",
+          "args": [
+              "--runInBand",
+              "--no-coverage",
+              // needs the '' around it so that the () are properly handled
+              "'tests/(.+/)?${fileBasenameNoExtension}'"
+          ],
+          "sourceMaps": true,
+          "console": "integratedTerminal",
+          "internalConsoleOptions": "neverOpen"
+      },
       {
         "type": "node",
         "request": "launch",

packages/eslint-plugin-internal/README.md

@@ -0,0 +1,5 @@
+# `eslint-plugin-internal`
+
+This is just a collection of internal lint rules to help enforce some guidelines specific to this repository.
+
+These are not intended to be used externally.

packages/eslint-plugin-internal/jest.config.js

@@ -0,0 +1,13 @@
+'use strict';
+
+module.exports = {
+  testEnvironment: 'node',
+  transform: {
+    '^.+\\.tsx?$': 'ts-jest',
+  },
+  testRegex: './tests/.+\\.test\\.ts$',
+  collectCoverage: false,
+  collectCoverageFrom: ['src/**/*.{js,jsx,ts,tsx}'],
+  moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'],
+  coverageReporters: ['text-summary', 'lcov'],
+};

packages/eslint-plugin-internal/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "@typescript-eslint/eslint-plugin-internal",
+  "version": "2.13.0",
+  "private": true,
+  "main": "dist/index.js",
+  "scripts": {
+    "build": "tsc -b tsconfig.build.json",
+    "clean": "tsc -b tsconfig.build.json --clean",
+    "format": "prettier --write \"./**/*.{ts,js,json,md}\" --ignore-path ../../.prettierignore",
+    "lint": "eslint . --ext .js,.ts --ignore-path='../../.eslintignore'",
+    "test": "jest --coverage",
+    "typecheck": "tsc -p tsconfig.json --noEmit"
+  },
+  "dependencies": {
+    "@typescript-eslint/experimental-utils": "2.13.0"
+  },
+  "devDependencies": {}
+}

packages/eslint-plugin-internal/src/index.ts

@@ -0,0 +1,5 @@
+import rules from './rules';
+
+export = {
+  rules,
+};

packages/eslint-plugin-internal/src/rules/index.ts

@@ -0,0 +1,7 @@
+import noTypescriptDefaultImport from './no-typescript-default-import';
+import noTypescriptEstreeImport from './no-typescript-estree-import';
+
+export default {
+  'no-typescript-default-import': noTypescriptDefaultImport,
+  'no-typescript-estree-import': noTypescriptEstreeImport,
+};

packages/eslint-plugin-internal/src/rules/no-typescript-default-import.ts

@@ -0,0 +1,80 @@
+import {
+  TSESTree,
+  AST_NODE_TYPES,
+} from '@typescript-eslint/experimental-utils';
+import { createRule } from '../util';
+
+/*
+We have `allowSyntheticDefaultImports` turned on in this project, so there are two problems that arise:
+- TypeScript's auto import will suggest `import ts = require('typescript');` if you type `ts`
+- VSCode's suggestion feature will suggest changing `import * as ts from 'typescript'` to `import ts from 'typescript'`
+
+In order to keep compatibility with a wide range of consumers, some of whom don't use `allowSyntheticDefaultImports`, we should
+always use either:
+- `import * as ts from 'typescript';`
+- `import { SyntaxKind } from 'typescript';`
+*/
+
+export default createRule({
+  name: 'no-typescript-default-import',
+  meta: {
+    type: 'problem',
+    docs: {
+      description:
+        "Enforces that packages rules don't do `import ts from 'typescript';`",
+      category: 'Possible Errors',
+      recommended: 'error',
+    },
+    fixable: 'code',
+    schema: [],
+    messages: {
+      noTSDefaultImport: [
+        "Do not use the default import for typescript. Doing so will cause the package's type definitions to do the same.",
+        "This causes errors for consumers if they don't use the allowSyntheticDefaultImports compiler option.",
+      ].join('\n'),
+    },
+  },
+  defaultOptions: [],
+  create(context) {
+    return {
+      'ImportDeclaration > ImportDefaultSpecifier'(
+        node: TSESTree.ImportDefaultSpecifier,
+      ): void {
+        const importStatement = node.parent as TSESTree.ImportDeclaration;
+        if (importStatement.source.value === 'typescript') {
+          context.report({
+            node,
+            messageId: 'noTSDefaultImport',
+            fix(fixer) {
+              if (importStatement.specifiers.length === 1) {
+                return fixer.replaceText(node, '* as ts');
+              }
+
+              return null;
+            },
+          });
+        }
+      },
+      'TSImportEqualsDeclaration > TSExternalModuleReference'(
+        node: TSESTree.TSExternalModuleReference,
+      ): void {
+        const parent = node.parent as TSESTree.TSImportEqualsDeclaration;
+        if (
+          node.expression.type === AST_NODE_TYPES.Literal &&
+          node.expression.value === 'typescript'
+        ) {
+          context.report({
+            node,
+            messageId: 'noTSDefaultImport',
+            fix(fixer) {
+              return fixer.replaceText(
+                parent,
+                "import * as ts from 'typescript';",
+              );
+            },
+          });
+        }
+      },
+    };
+  },
+});

packages/eslint-plugin-internal/src/rules/no-typescript-estree-import.ts

@@ -0,0 +1,52 @@
+import { createRule } from '../util';
+
+const TSESTREE_NAME = '@typescript-eslint/typescript-estree';
+const UTILS_NAME = '@typescript-eslint/experimental-utils';
+
+/*
+Typescript will not error if people use typescript-estree within eslint-plugin.
+This is because it's an indirect dependency.
+We don't want people to import it, instead we want them to import from the utils package.
+*/
+
+export default createRule({
+  name: 'no-typescript-estree-import',
+  meta: {
+    type: 'problem',
+    docs: {
+      description: `Enforces that eslint-plugin rules don't require anything from ${TSESTREE_NAME}`,
+      category: 'Possible Errors',
+      recommended: 'error',
+    },
+    fixable: 'code',
+    schema: [],
+    messages: {
+      dontImportTSEStree: [
+        `Don't import from ${TSESTREE_NAME}. Everything you need should be available in ${UTILS_NAME}.`,
+        `${TSESTREE_NAME} is an indirect dependency of this package, and thus should not be used directly.`,
+      ].join('\n'),
+    },
+  },
+  defaultOptions: [],
+  create(context) {
+    return {
+      ImportDeclaration(node): void {
+        if (
+          typeof node.source.value === 'string' &&
+          node.source.value.startsWith(TSESTREE_NAME)
+        ) {
+          context.report({
+            node,
+            messageId: 'dontImportTSEStree',
+            fix(fixer) {
+              return fixer.replaceTextRange(
+                [node.source.range[0] + 1, node.source.range[1] - 1],
+                UTILS_NAME,
+              );
+            },
+          });
+        }
+      },
+    };
+  },
+});

packages/eslint-plugin-internal/src/util/createRule.ts

@@ -0,0 +1,11 @@
+import { ESLintUtils } from '@typescript-eslint/experimental-utils';
+
+// note - cannot migrate this to an import statement because it will make TSC copy the package.json to the dist folder
+const version = require('../../package.json').version;
+
+const createRule = ESLintUtils.RuleCreator(
+  name =>
+    `https://github.com/typescript-eslint/typescript-eslint/blob/v${version}/packages/eslint-plugin-internal/src/rules/${name}.ts`,
+);
+
+export { createRule };

packages/eslint-plugin-internal/src/util/index.ts

@@ -0,0 +1 @@
+export * from './createRule';

packages/eslint-plugin-internal/tests/RuleTester.ts

@@ -0,0 +1,22 @@
+import { TSESLint, ESLintUtils } from '@typescript-eslint/experimental-utils';
+
+const { batchedSingleLineTests } = ESLintUtils;
+
+const parser = '@typescript-eslint/parser';
+
+type RuleTesterConfig = Omit<TSESLint.RuleTesterConfig, 'parser'> & {
+  parser: typeof parser;
+};
+class RuleTester extends TSESLint.RuleTester {
+  // as of eslint 6 you have to provide an absolute path to the parser
+  // but that's not as clean to type, this saves us trying to manually enforce
+  // that contributors require.resolve everything
+  constructor(options: RuleTesterConfig) {
+    super({
+      ...options,
+      parser: require.resolve(options.parser),
+    });
+  }
+}
+
+export { RuleTester, batchedSingleLineTests };

packages/eslint-plugin-internal/tests/rules/no-typescript-default-import.test.ts

@@ -0,0 +1,45 @@
+import rule from '../../src/rules/no-typescript-default-import';
+import { RuleTester, batchedSingleLineTests } from '../RuleTester';
+
+const ruleTester = new RuleTester({
+  parser: '@typescript-eslint/parser',
+  parserOptions: {
+    sourceType: 'module',
+  },
+});
+
+ruleTester.run('no-typescript-default-import', rule, {
+  valid: [
+    "import { foo } from 'typescript';",
+    "import ts from 'nottypescript';",
+    "import * as foo from 'typescript';",
+    'import ts = foo;',
+    "import ts = require('nottypescript');",
+  ],
+  invalid: batchedSingleLineTests({
+    code: `
+import ts from 'typescript';
+import ts, { SyntaxKind } from 'typescript';
+import ts = require('typescript');
+    `,
+    output: `
+import * as ts from 'typescript';
+import ts, { SyntaxKind } from 'typescript';
+import * as ts from 'typescript';
+    `,
+    errors: [
+      {
+        messageId: 'noTSDefaultImport',
+        line: 2,
+      },
+      {
+        messageId: 'noTSDefaultImport',
+        line: 3,
+      },
+      {
+        messageId: 'noTSDefaultImport',
+        line: 4,
+      },
+    ],
+  }),
+});