From 98f71588bb08cb3933c2fb86f4d70dc1e8a7df24 Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Mon, 9 Jul 2018 13:03:56 +0300 Subject: [PATCH 1/2] Implement 'no-default-import' rule --- src/configs/all.ts | 1 + src/rules/noDefaultImportRule.ts | 128 ++++++++++++++++++ .../no-default-import/default/test.ts.lint | 36 +++++ .../no-default-import/default/tslint.json | 5 + .../fromModules/test.ts.lint | 24 ++++ .../no-default-import/fromModules/tslint.json | 10 ++ 6 files changed, 204 insertions(+) create mode 100644 src/rules/noDefaultImportRule.ts create mode 100644 test/rules/no-default-import/default/test.ts.lint create mode 100644 test/rules/no-default-import/default/tslint.json create mode 100644 test/rules/no-default-import/fromModules/test.ts.lint create mode 100644 test/rules/no-default-import/fromModules/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index 142bc1426b6..aed7806a45f 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -153,6 +153,7 @@ export const rules = { "max-file-line-count": [true, 1000], "max-line-length": [true, 120], "no-default-export": true, + "no-default-import": true, "no-duplicate-imports": true, "no-irregular-whitespace": true, "no-mergeable-namespace": true, diff --git a/src/rules/noDefaultImportRule.ts b/src/rules/noDefaultImportRule.ts new file mode 100644 index 00000000000..7c7301fcae6 --- /dev/null +++ b/src/rules/noDefaultImportRule.ts @@ -0,0 +1,128 @@ +/** + * @license + * Copyright 2016 Palantir Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isImportDeclaration, isNamedImports, isStringLiteral } from "tsutils"; +import * as ts from "typescript"; +import * as Lint from "../index"; + +const fromModulesConfigOptionName = "fromModules"; +interface RawConfigOptions { + [fromModulesConfigOptionName]: string; +} +interface Options { + [fromModulesConfigOptionName]: RegExp; +} + +export class Rule extends Lint.Rules.AbstractRule { + /* tslint:disable:object-literal-sort-keys */ + public static metadata: Lint.IRuleMetadata = { + ruleName: "no-default-import", + description: "Disallows importing default members from certain ES6-style modules.", + descriptionDetails: "Import named members instead.", + rationale: Lint.Utils.dedent` + Named imports/exports [promote clarity](https://github.com/palantir/tslint/issues/1182#issue-151780453). + In addition, current tooling differs on the correct way to handle default imports/exports. + Avoiding them all together can help avoid tooling bugs and conflicts. + + The rule supposed to narrow the scope of your changes in the case of monorepos. + Say, you have 20 packages and every removed default export from utility package would lead to changes + in each package, which might be harder to get merged by various reasons (harder to get your code approved + due to a number of required reviewers; longer build time due to a number of affected packages). + That's why "requires too many changes elsewhere" is a reason to ignore "no-default-export" rule. + + Unlike "no-default-export", the rule requires you to make changes only in the package you work on + and the package you import from (unless the member you try to import already exported as a named one).`, + optionsDescription: "optionsDescription", + options: { + type: "array", + items: { + type: "object", + properties: { + [fromModulesConfigOptionName]: { type: "string" }, + }, + required: [ + "fromModules", + ], + }, + }, + optionExamples: [ + [true, { [fromModulesConfigOptionName]: "^palantir-|^_internal-*|^\\./|^\\.\\./" }], + ], + type: "maintainability", + typescriptOnly: false, + }; + /* tslint:enable:object-literal-sort-keys */ + + public static FAILURE_STRING = "Import of default members from this module is forbidden. Import named member instead"; + + public static getNamedDefaultImport(namedBindings: ts.NamedImports): ts.Identifier | null { + for (const importSpecifier of namedBindings.elements) { + if (importSpecifier.propertyName !== undefined && importSpecifier.propertyName.text === "default") { + return importSpecifier.propertyName; + } + } + return null; + } + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk, this.getRuleOptions(this.ruleArguments)); + } + private isFromModulesConfigOption(option: boolean | RawConfigOptions): option is RawConfigOptions { + return typeof option === "object" && option[fromModulesConfigOptionName] !== undefined; + } + private getRuleOptions(options: ReadonlyArray): Options { + const fromModuleConfigOption = options.find(this.isFromModulesConfigOption); + if (fromModuleConfigOption !== undefined && typeof fromModuleConfigOption[fromModulesConfigOptionName] === "string") { + return { + [fromModulesConfigOptionName]: new RegExp(fromModuleConfigOption[fromModulesConfigOptionName]), + }; + } else { + return { + [fromModulesConfigOptionName]: new RegExp("^\\./|^\\.\\./"), + }; + } + } +} + +function walk(ctx: Lint.WalkContext) { + if (ctx.sourceFile.isDeclarationFile || !ts.isExternalModule(ctx.sourceFile)) { + return; + } + for (const statement of ctx.sourceFile.statements) { + if (isImportDeclaration(statement)) { + const { importClause, moduleSpecifier } = statement; + if ( + importClause !== undefined + && isStringLiteral(moduleSpecifier) + && ctx.options[fromModulesConfigOptionName].test(moduleSpecifier.text) + ) { + // module name matches specified in rule config + if (importClause.name !== undefined) { + // `import Foo...` syntax + const defaultImportedName = importClause.name; + ctx.addFailureAtNode(defaultImportedName, Rule.FAILURE_STRING); + } else if (importClause.namedBindings !== undefined && isNamedImports(importClause.namedBindings)) { + // `import { default...` syntax + const defaultMember = Rule.getNamedDefaultImport(importClause.namedBindings); + if (defaultMember !== null) { + ctx.addFailureAtNode(defaultMember, Rule.FAILURE_STRING); + } + } + } + } + } +} diff --git a/test/rules/no-default-import/default/test.ts.lint b/test/rules/no-default-import/default/test.ts.lint new file mode 100644 index 00000000000..399593ed9f6 --- /dev/null +++ b/test/rules/no-default-import/default/test.ts.lint @@ -0,0 +1,36 @@ +import * as Utils from "tslint-utils" + +import TslintUtils from "tslint-utils" + +import Bar, { Foo } from "tslint-misc" + +import Bar, * as Foo from "tslint-misc" + +import { default as Foo } from "tslint-misc" + +import { default as foo, bar } from "tslint-misc" + +import { bar, default as foo } from "tslint-misc" + +import TslintUtils from "../tslint-utils" + ~~~~~~~~~~~ [0] + +import TslintUtils from "./tslint-utils" + ~~~~~~~~~~~ [0] + +import Bar, { Foo } from "../tslint-misc" + ~~~ [0] + +import Bar, * as Foo from "./tslint-misc" + ~~~ [0] + +import { default as Foo } from "../tslint-misc" + ~~~~~~~ [0] + +import { default as foo, bar } from "./tslint-misc" + ~~~~~~~ [0] + +import { bar, default as foo } from "../tslint-misc" + ~~~~~~~ [0] + +[0]: Import of default members from this module is forbidden. Import named member instead diff --git a/test/rules/no-default-import/default/tslint.json b/test/rules/no-default-import/default/tslint.json new file mode 100644 index 00000000000..63855417715 --- /dev/null +++ b/test/rules/no-default-import/default/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "no-default-import": true + } +} diff --git a/test/rules/no-default-import/fromModules/test.ts.lint b/test/rules/no-default-import/fromModules/test.ts.lint new file mode 100644 index 00000000000..2b789830c6e --- /dev/null +++ b/test/rules/no-default-import/fromModules/test.ts.lint @@ -0,0 +1,24 @@ +import * as Utils from "tslint-utils" + +import TslintUtils from "../tslint-utils" + ~~~~~~~~~~~ [0] + +import TslintUtils from "tslint-utils" + ~~~~~~~~~~~ [0] + +import Bar, { Foo } from "tslint-misc" + ~~~ [0] + +import Bar, * as Foo from "tslint-misc" + ~~~ [0] + +import { default as Foo } from "tslint-misc" + ~~~~~~~ [0] + +import { default as foo, bar } from "tslint-misc" + ~~~~~~~ [0] + +import { bar, default as foo } from "tslint-misc" + ~~~~~~~ [0] + +[0]: Import of default members from this module is forbidden. Import named member instead diff --git a/test/rules/no-default-import/fromModules/tslint.json b/test/rules/no-default-import/fromModules/tslint.json new file mode 100644 index 00000000000..1d9bf38d79d --- /dev/null +++ b/test/rules/no-default-import/fromModules/tslint.json @@ -0,0 +1,10 @@ +{ + "rules": { + "no-default-import": [ + true, + { + "fromModules": "^tslint-|^\\./|^\\.\\./" + } + ] + } +} From 70269a1a159166a23c4a16e69108a514c7ecd7b5 Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Mon, 3 Sep 2018 19:12:27 +0300 Subject: [PATCH 2/2] Improve wording in rationale --- src/rules/noDefaultImportRule.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/rules/noDefaultImportRule.ts b/src/rules/noDefaultImportRule.ts index 7c7301fcae6..f242e60d142 100644 --- a/src/rules/noDefaultImportRule.ts +++ b/src/rules/noDefaultImportRule.ts @@ -38,14 +38,16 @@ export class Rule extends Lint.Rules.AbstractRule { In addition, current tooling differs on the correct way to handle default imports/exports. Avoiding them all together can help avoid tooling bugs and conflicts. - The rule supposed to narrow the scope of your changes in the case of monorepos. - Say, you have 20 packages and every removed default export from utility package would lead to changes - in each package, which might be harder to get merged by various reasons (harder to get your code approved - due to a number of required reviewers; longer build time due to a number of affected packages). - That's why "requires too many changes elsewhere" is a reason to ignore "no-default-export" rule. + The rule supposed to narrow the scope of your changes in the case of monorepo. + Say, you have packages \`A\`, \`B\`, \`C\` and \`utils\`, where \`A\`, \`B\`, \`C\` dependends on \`utils\`, + which is full of default exports. + \`"no-default-export"\` requires you to remove default _export_ from \`utils\`, which leads to changes + in packages \`A\`, \`B\`, \`C\`. It's harder to get merged bigger changeset by various reasons (harder to get your code approved + due to a number of required reviewers; longer build time due to a number of affected packages) + and could result in ignored \`"no-default-export"\` rule in \`utils'\`. - Unlike "no-default-export", the rule requires you to make changes only in the package you work on - and the package you import from (unless the member you try to import already exported as a named one).`, + Unlike \`"no-default-export"\`, the rule requires you to repalce default _import_ with named only in \`A\` you work on, + and \`utils\` you import from.`, optionsDescription: "optionsDescription", options: { type: "array",