Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add no-submodule-imports rule #3091

Merged
merged 15 commits into from
Aug 7, 2017
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@
},

// Always use project's provided typescript compiler version
"typescript.tsdk": "node_modules/typescript/lib"
"typescript.tsdk": "node_modules/typescript/lib",
"files.eol": "\n"
}
1 change: 1 addition & 0 deletions src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export const rules = {
"no-string-literal": true,
"no-string-throw": true,
"no-sparse-arrays": true,
"no-submodule-imports": true,
"no-unbound-method": true,
"no-unsafe-any": true,
"no-unsafe-finally": true,
Expand Down
115 changes: 115 additions & 0 deletions src/rules/noSubmoduleImportsRule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/**
* @license
* Copyright 2017 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 {
isCallExpression,
isExternalModuleReference,
isIdentifier,
isImportDeclaration,
isImportEqualsDeclaration,
isTextualLiteral,
} from "tsutils";
import * as ts from "typescript";
import * as Lint from "../index";

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
ruleName: "no-submodule-imports",
description: Lint.Utils.dedent`
Disallows importing any submodule.`,
rationale: Lint.Utils.dedent`
Submodules of some packages are treated as private APIs and the import
paths may change without deprecation periods. It's best to stick with
top-level package exports.`,
optionsDescription: "A list of packages whose submodules are whitelisted.",
options: {
type: "array",
items: {
type: "string",
},
minLength: 0,
},
optionExamples: [true, [true, "rxjs", "@angular/core"]],
type: "functionality",
typescriptOnly: false,
};

public static FAILURE_STRING = "Submodule import paths from this package are disallowed; import from the root instead";

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithWalker(new NoSubmoduleImportsWalker(sourceFile, this.ruleName, this.ruleArguments));
}
}

class NoSubmoduleImportsWalker extends Lint.AbstractWalker<string[]> {
public walk(sourceFile: ts.SourceFile) {
const findDynamicImport = (node: ts.Node): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't need to be a const, please make it a plain helper function at the end of the file

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't. It uses the lexical this binding

if (isCallExpression(node) && node.arguments.length === 1 &&
(isIdentifier(node.expression) && node.expression.text === "require" ||
node.expression.kind === ts.SyntaxKind.ImportKeyword)) {
this.checkForBannedImport(node.arguments[0]);
}
return ts.forEachChild(node, findDynamicImport);
};

for (const statement of sourceFile.statements) {
if (isImportDeclaration(statement)) {
this.checkForBannedImport(statement.moduleSpecifier);
} else if (isImportEqualsDeclaration(statement)) {
if (isExternalModuleReference(statement.moduleReference) && statement.moduleReference.expression !== undefined) {
this.checkForBannedImport(statement.moduleReference.expression);
}
} else {
ts.forEachChild(statement, findDynamicImport);
}
}
}

private checkForBannedImport(expression: ts.Expression) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a static function (doesn't use this), you can make it static or move to a plain helper function

Copy link
Contributor

Choose a reason for hiding this comment

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

Only if this addFailureAtNode is moved out of the function. This one should stay as is.

if (isTextualLiteral(expression)) {
if (isAbsoluteOrRelativePath(expression.text) || !isSubmodulePath(expression.text)) {
return;
}

/**
* A submodule is being imported.
* Check if its path contains any
* of the whitelist packages.
*/
for (const option of this.options) {
if (expression.text.startsWith(`${option}/`)) {
return;
}
}

this.addFailureAtNode(expression, Rule.FAILURE_STRING);
}
}
}

function isAbsoluteOrRelativePath(path: string): boolean {
return /^(..?(\/|$)|\/)/.test(path);
}

function isScopedPath(path: string): boolean {
return path[0] === "@";
}

function isSubmodulePath(path: string): boolean {
return path.split("/").length > (isScopedPath(path) ? 2 : 1);
}
23 changes: 23 additions & 0 deletions test/rules/no-submodule-imports/dynamic-imports/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const dynamicImport = import "lodash";
Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually const dynamicImport = import("lodash");


const dynamicImport = import "lodash/sub-module";
~~~~~~~~~~~~~~~~~~~ [0]

const dynamicImport = import "lodash/a/b/c/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~ [0]


const dynamicImport = import "@blueprintjs/core";

const dynamicImport = import "@blueprintjs/core/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

const dynamicImport = import "@blueprintjs/core/a/b/c/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

const dynamicImport = import "rxjs";
const dynamicImport = import "rxjs/sub-module";
const dynamicImport = import "@angular/core";
const dynamicImport = import "@angular/core/forms/directives";

[0]: Submodule import paths from this package are disallowed; import from the root instead
5 changes: 5 additions & 0 deletions test/rules/no-submodule-imports/dynamic-imports/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"no-submodule-imports": [true, "@angular/core", "rxjs"]
}
}
61 changes: 61 additions & 0 deletions test/rules/no-submodule-imports/static-imports/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { submodule } from "@blueprintjs/core";

import { submodule } from "@blueprintjs/core/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import submodule = require("@blueprintjs/core");

import submodule = require("@blueprintjs/core/sub-module");
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import { submodule } from "@blueprintjs/core";

import { submodule } from "@blueprintjs/core/a/b/c/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import submodule = require("@blueprintjs/core");

import submodule = require("@blueprintjs/core/a/b/c/sub-module");
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import { submodule } from "@angular/core";
import { submodule } from "@angular/core/sub-module";
import submodule = require("@angular/core");
import submodule = require("@angular/core/sub-module");
import { submodule } from "@angular/core";
import { submodule } from "@angular/core/a/b/c/sub-module";
import submodule = require("@angular/core");
import submodule = require("@angular/core/a/b/c/sub-module");


import { submodule } from "lodash";

import { submodule } from "lodash/sub-module";
~~~~~~~~~~~~~~~~~~~ [0]

import submodule = require("lodash");

import submodule = require("lodash/sub-module");
~~~~~~~~~~~~~~~~~~~ [0]

import { submodule } from "lodash";

import { submodule } from "lodash/a/b/c/sub-module";
~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import submodule = require("lodash");

import submodule = require("lodash/a/b/c/sub-module");
~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

import { submodule } from "rxjs";
import { submodule } from "rxjs/sub-module";
import submodule = require("rxjs");
import submodule = require("rxjs/sub-module");
import { submodule } from "rxjs";
import { submodule } from "rxjs/a/b/c/sub-module";
import submodule = require("rxjs");
import submodule = require("rxjs/a/b/c/sub-module");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test with a relative path



[0]: Submodule import paths from this package are disallowed; import from the root instead
5 changes: 5 additions & 0 deletions test/rules/no-submodule-imports/static-imports/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"no-submodule-imports": [true, "@angular/core", "rxjs"]
}
}