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
Merged

Add no-submodule-imports rule #3091

merged 15 commits into from
Aug 7, 2017

Conversation

aervin
Copy link
Contributor

@aervin aervin commented Aug 2, 2017

Attempt at the functionality requested here: #3051

This essentially extends the import-blacklist rule with a custom implementation of checkForBannedImport().

optionsDescription: A list of modules whose submodules are blocked from being imported directly.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @aervin! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@aervin
Copy link
Contributor Author

aervin commented Aug 2, 2017

Fixing CI issue now

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

most importantly: can you please use the rule name I suggested in the original issue, no-submodule-imports?

After thinking about it some more, I think it should ban submodule imports from all external packages by default, and optionally accept a whitelist of modules where such imports are allowed.

*/

import {
isCallExpression, isExternalModuleReference, isIdentifier, isImportDeclaration, isImportEqualsDeclaration, isTextualLiteral,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please split these imports into multiple lines

description: Lint.Utils.dedent`
Disallows importing any submodule of the listed modules.`,
rationale: Lint.Utils.dedent`
Not sure on this exactly...`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Try this:

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.

Disallows importing any submodule of the listed modules.`,
rationale: Lint.Utils.dedent`
Not sure on this exactly...`,
optionsDescription: "A list of modules whose submodules are blacklisted.",
Copy link
Contributor

Choose a reason for hiding this comment

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

"A list of packages whose submodules ..."

typescriptOnly: false,
};

public static FAILURE_STRING = "This submodule is blacklisted, try importing from a parent module";
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

"Submodule import paths from this package are disallowed; import from the root instead"


public static FAILURE_STRING = "This submodule is blacklisted, try importing from a parent module";

public isEnabled(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to override this method, we don't do it elsewhere

if (isTextualLiteral(expression)) {
let blacklistOption = "";

this.options.forEach((option: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use a for-of loop, not forEach

@@ -0,0 +1,25 @@
import { submodule } from '@blueprintjs/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please use double quotes to be consistent with the default config presets (quotemark: [true, "double"])

~~~~~~~~~~~~~~~~~ [0]

// non-static imports cannot be checked
import {Observable} from rxjs;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this valid TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was confused by this as well. I relied heavily on the import-blacklist test code to get my test case up and running. Could use some clarification on this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I introduced this test case back then.
It's not valid typescript, but the parser consumes it. The AST can contain any expression as module specifier and tslint should not crash because it only expects string literals.

// non-static imports cannot be checked
import {Observable} from rxjs;
import forOwn = require(lodash);

Copy link
Contributor

Choose a reason for hiding this comment

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

can you test dynamic import syntax too? You'll probably need to make a new test file that applies to TS 2.4 only (so CI knows not to run the test with TS parser <v2.4). see an example here

[typescript]: >=2.4.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like:
const forOwn = require(lodash).forOwn?

@adidahiya
Copy link
Contributor

also, can you please remove the package-lock.json file? We use yarnpkg lockfiles instead.

@aervin
Copy link
Contributor Author

aervin commented Aug 4, 2017

@adidahiya thanks for all the feedback! I'll work on this later today. May ask for further guidance if that's alright.

public walk(sourceFile: ts.SourceFile) {
const findRequire = (node: ts.Node): void => {
if (isCallExpression(node) && node.arguments.length === 1 &&
isIdentifier(node.expression) && node.expression.text === "require") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add || node.expression.kind === ts.SyntaxKind.ImportKeyword here to make it work with dynamic imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS is yelling at me unless I use node.expression.originalKeywordKind--I was grouping w/ node.expression.text === "require" like so:
isIdentifier(node.expression) && (node.expression.text === "require" || node.expression.originalKeywordKind === ts.SyntaxKind.ImportKeyword)

Am I way off here?

Copy link
Contributor

Choose a reason for hiding this comment

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

(isIdentifier(node.expression) && node.expression.text === "require" || node.expression.kind === ts.SyntaxKind.ImportKeyword)

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 blacklisted.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider making this a whitelist and ban all submodule imports by default.
That means we're only interested in imports that are no relative or absolute paths: !/^(..?(\/|$)|\/)/.test(path)
It should handle scoped and unscoped package names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay great, will tackle it sometime this weekend. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And just to clarify: you mean that import { forOwn } from "lodash"; and import { Something } from "@angular/core"; would both be allowed by default, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, those are always allowed. And import * as forOwn from "lodash/forown"; as well as import { NgModel } from "@angular/forms/src/directives" would be banned by default unless lodash or @angular/forms are whitelisted respectively.

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

You have modified the file permissions of every single file in this repo. That needs to be reverted.

const scopedPath = (path: string): boolean => {
return path[0] === "@";
};
const submodulePath = (path: string): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move those functions out of this method and make them function declarations. Also prepend is to each name

@@ -0,0 +1,133 @@
/**
* @license
* Copyright 2016 Palantir Technologies, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

public static metadata: Lint.IRuleMetadata = {
ruleName: "no-submodule-imports",
description: Lint.Utils.dedent`
Disallows importing any submodule of the listed modules.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove of the listed modules

items: {
type: "string",
},
minLength: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

0


class NoSubmoduleImportsWalker extends Lint.AbstractWalker<string[]> {
public walk(sourceFile: ts.SourceFile) {
const findRequire = (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.

Better name would be findDynamicImport

let whitelisted = false;

for (const option of this.options) {
if (expression.text.indexOf(option) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

`.startsWith(option +"/")


for (const option of this.options) {
if (expression.text.indexOf(option) !== -1) {
whitelisted = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return here


if (whitelisted) { return; }

this.addFailure(
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply use addFailureAtNode(expression, Rule.FAILURE_STRING)

@@ -117,6 +117,7 @@ export const rules = {
"no-string-literal": true,
"no-string-throw": true,
"no-sparse-arrays": true,
// "no-submodule-imports": no sensible default
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be enabled by default

@aervin
Copy link
Contributor Author

aervin commented Aug 6, 2017

I'll tackle these latest change requests this morning. @ajafff do I need to add some additional test coverage for dynamic imports?

@aervin
Copy link
Contributor Author

aervin commented Aug 6, 2017

Help @ajafff! CI build error is beyond my control I think. Needs retriggered?

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

I left some final suggestions. These are not required, so you can choose to ignore them.

private checkForBannedImport(expression: ts.Expression) {
if (isTextualLiteral(expression)) {
if (isAbsoluteOrRelativePath(expression.text)) { return; }
if (!isSubmodulePath(expression.text)) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Both if statements can be combined with the outer if.

}
}

this.addFailureAtNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be written in one line

}

function isSubmodulePath(path: string): boolean {
const pathDepth: number = path.split("/").length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified to

return path.split("/").length > (isScopedPath(path) ? 2 : 1);

@ajafff
Copy link
Contributor

ajafff commented Aug 6, 2017

CI is very flaky lately. I restarted it.

There's still the problem with the changed file permissions. Have a look at the Changes tab of this PR to see what I mean. I can't see your tests because there are too many changed files to display.

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,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");

@aervin
Copy link
Contributor Author

aervin commented Aug 7, 2017

I think I've addressed all requested changes thus far. Happy to make further edits, however.

@ajafff
Copy link
Contributor

ajafff commented Aug 7, 2017

Sorry for the delay. I'm currently on vacation and don't have that much time to offer and mobile internet connection is pretty bad.
However, the code looks good to me. Thanks for your great work.

We may also want to enable this rule in the tslint:latest preset.

@aervin
Copy link
Contributor Author

aervin commented Aug 7, 2017

Don't let me bug you @ajafff-- have a great vacation and thanks for all your help!

@adidahiya adidahiya changed the title First pass at new rule proposal #3051: import-blacklist-submodules Add no-submodule imports rule Aug 7, 2017
@adidahiya adidahiya changed the title Add no-submodule imports rule Add no-submodule-imports rule Aug 7, 2017
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

looks good; just some minor stylistic comments that can be addressed in a follow-up


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

}
}

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.

@adidahiya
Copy link
Contributor

adidahiya commented Aug 7, 2017

I'm going to merge this and look into the minor style comments + tslint:latest separately.

@adidahiya adidahiya merged commit a4b0642 into palantir:master Aug 7, 2017
@aervin
Copy link
Contributor Author

aervin commented Aug 7, 2017

Can open a fresh PR for those updates later tonight. Do you imagine many more import-related rules coming along in future? If so, might be nice to keep a template/list of all the different import syntax possibilities that TsLint supports/tests. Becomes a sort of copy and paste job for anyone who needs to test their import-related rule.

@ajafff
Copy link
Contributor

ajafff commented Aug 7, 2017

@aervin I'm planning to add a function to tsutils that finds all imports in a file. That will reduce code duplication in tslint rules

@aervin
Copy link
Contributor Author

aervin commented Aug 7, 2017

@ajafff that would help a very great deal. I was referring to what would go in an import rule's test.ts.lint in the above comment. I just found it tedious to make sure I was covering all bases in the rule's tests.

Also, we've talked about moving away from relative imports at work. no-relative-imports rule might be useful?

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants