Skip to content
Permalink
Browse files

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.
  • Loading branch information
bradzacher committed Dec 24, 2019
1 parent a78b194 commit 3a15413d87a3429ebf19af2cc5db76c9e7ffe4e7
Showing with 401 additions and 47 deletions.
  1. +19 −5 .eslintrc.js
  2. +16 −0 .vscode/launch.json
  3. +5 −0 packages/eslint-plugin-internal/README.md
  4. +13 −0 packages/eslint-plugin-internal/jest.config.js
  5. +18 −0 packages/eslint-plugin-internal/package.json
  6. +5 −0 packages/eslint-plugin-internal/src/index.ts
  7. +7 −0 packages/eslint-plugin-internal/src/rules/index.ts
  8. +80 −0 packages/eslint-plugin-internal/src/rules/no-typescript-default-import.ts
  9. +52 −0 packages/eslint-plugin-internal/src/rules/no-typescript-estree-import.ts
  10. +11 −0 packages/eslint-plugin-internal/src/util/createRule.ts
  11. +1 −0 packages/eslint-plugin-internal/src/util/index.ts
  12. +22 −0 packages/eslint-plugin-internal/tests/RuleTester.ts
  13. +45 −0 packages/eslint-plugin-internal/tests/rules/no-typescript-default-import.test.ts
  14. +43 −0 packages/eslint-plugin-internal/tests/rules/no-typescript-estree.test.ts
  15. +13 −0 packages/eslint-plugin-internal/tsconfig.build.json
  16. +8 −0 packages/eslint-plugin-internal/tsconfig.json
  17. +1 −1 packages/eslint-plugin/src/rules/await-thenable.ts
  18. +1 −1 packages/eslint-plugin/src/rules/no-for-in-array.ts
  19. +1 −1 packages/eslint-plugin/src/rules/no-misused-promises.ts
  20. +7 −5 packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
  21. +1 −1 packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts
  22. +1 −1 packages/eslint-plugin/src/rules/no-unnecessary-type-arguments.ts
  23. +1 −1 packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
  24. +1 −1 packages/eslint-plugin/src/rules/no-unused-vars-experimental.ts
  25. +1 −1 packages/eslint-plugin/src/rules/prefer-includes.ts
  26. +1 −1 packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
  27. +1 −1 packages/eslint-plugin/src/rules/prefer-readonly.ts
  28. +1 −1 packages/eslint-plugin/src/rules/require-array-sort-compare.ts
  29. +1 −1 packages/eslint-plugin/src/rules/require-await.ts
  30. +1 −1 packages/eslint-plugin/src/rules/restrict-plus-operands.ts
  31. +1 −1 packages/eslint-plugin/src/rules/restrict-template-expressions.ts
  32. +2 −2 packages/eslint-plugin/src/rules/return-await.ts
  33. +1 −1 packages/eslint-plugin/src/rules/strict-boolean-expressions.ts
  34. +1 −1 packages/eslint-plugin/src/rules/unbound-method.ts
  35. +1 −1 packages/eslint-plugin/src/util/types.ts
  36. +0 −1 packages/experimental-utils/package.json
  37. +1 −1 packages/typescript-estree/src/convert-comments.ts
  38. +1 −1 packages/typescript-estree/src/convert.ts
  39. +1 −1 packages/typescript-estree/src/create-program/WatchCompilerHostOfConfigFile.ts
  40. +1 −1 packages/typescript-estree/src/create-program/createDefaultProgram.ts
  41. +1 −1 packages/typescript-estree/src/create-program/createIsolatedProgram.ts
  42. +1 −1 packages/typescript-estree/src/create-program/createSourceFile.ts
  43. +1 −1 packages/typescript-estree/src/create-program/createWatchProgram.ts
  44. +1 −1 packages/typescript-estree/src/create-program/shared.ts
  45. +1 −1 packages/typescript-estree/src/node-utils.ts
  46. +1 −1 packages/typescript-estree/src/parser.ts
  47. +1 −1 packages/typescript-estree/src/semantic-or-syntactic-errors.ts
  48. +1 −1 packages/typescript-estree/src/ts-estree/ts-nodes.ts
  49. +1 −1 packages/typescript-estree/tests/lib/convert.ts
  50. +1 −1 packages/typescript-estree/tests/lib/semanticInfo.ts
  51. +3 −3 yarn.lock
@@ -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: {
@@ -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",
@@ -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.
@@ -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'],
};
@@ -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": {}
}
@@ -0,0 +1,5 @@
import rules from './rules';

export = {
rules,
};
@@ -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,
};
@@ -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';",
);
},
});
}
},
};
},
});
@@ -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,
);
},
});
}
},
};
},
});
@@ -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 };
@@ -0,0 +1 @@
export * from './createRule';
@@ -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 };
@@ -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,
},
],
}),
});

0 comments on commit 3a15413

Please sign in to comment.
You can’t perform that action at this time.