-
Notifications
You must be signed in to change notification settings - Fork 888
import-blacklist: support banning the import of specific named exports #3926
Changes from 2 commits
d18175c
692c0aa
c9581f7
3c3eb40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
import { findImports, ImportKind } from "tsutils"; | ||
import { findImports, ImportKind, isExportDeclaration, isImportDeclaration, isNamedImports } from "tsutils"; | ||
import * as ts from "typescript"; | ||
import * as Lint from "../index"; | ||
|
||
|
@@ -24,25 +24,59 @@ export class Rule extends Lint.Rules.AbstractRule { | |
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "import-blacklist", | ||
description: Lint.Utils.dedent` | ||
Disallows importing the specified modules directly via \`import\` and \`require\`. | ||
Instead only sub modules may be imported from that module.`, | ||
Disallows importing the specified modules via \`import\` and \`require\`, | ||
or importing specific named exports of the specified modules.`, | ||
rationale: Lint.Utils.dedent` | ||
Some libraries allow importing their submodules instead of the entire module. | ||
This is good practise as it avoids loading unused modules.`, | ||
optionsDescription: "A list of blacklisted modules.", | ||
For some libraries, importing the library directly can cause unused | ||
submodules to be loaded, so you may want to block these imports and | ||
require that users directly import only the submodules they need. | ||
In other cases, you may simply want to ban an import because using | ||
it is undesirable or unsafe.`, | ||
optionsDescription: "A list of blacklisted modules or named imports.", | ||
options: { | ||
type: "array", | ||
items: { | ||
type: "string", | ||
oneOf: [ | ||
{ | ||
type: "string", | ||
minLength: 1, | ||
}, | ||
{ | ||
type: "object", | ||
additionalProperties: { | ||
type: "array", | ||
minItems: 1, | ||
items: { | ||
type: "string", | ||
minLength: 1, | ||
}, | ||
}, | ||
}, | ||
], | ||
}, | ||
minLength: 1, | ||
}, | ||
optionExamples: [true, [true, "rxjs", "lodash"]], | ||
optionExamples: [ | ||
true, | ||
[true, "rxjs", "lodash"], | ||
[true, "lodash", { lodash: ["pull", "pullAll"] }], | ||
], | ||
type: "functionality", | ||
typescriptOnly: false, | ||
}; | ||
|
||
public static FAILURE_STRING = "This import is blacklisted, import a submodule instead"; | ||
public static WHOLE_MODULE_FAILURE_STRING = | ||
"Importing this module is blacklisted. Try importing a submodule instead."; | ||
|
||
public static IMPLICIT_NAMED_IMPORT_FAILURE_STRING = | ||
"Some named exports from this module are blacklisted for importing " + | ||
"(or re-exporting). Import/re-export only the specific values you want, " + | ||
"instead of the whole module."; | ||
|
||
public static MAKE_NAMED_IMPORT_FAILURE_STRING(importName: string) { | ||
return importName === "default" | ||
? "Importing (or re-exporting) the default export is blacklisted." | ||
: `The export "${importName}" is blacklisted.`; | ||
} | ||
|
||
public isEnabled(): boolean { | ||
return super.isEnabled() && this.ruleArguments.length > 0; | ||
|
@@ -54,9 +88,140 @@ export class Rule extends Lint.Rules.AbstractRule { | |
} | ||
|
||
function walk(ctx: Lint.WalkContext<string[]>) { | ||
type Options = Array<string | { [moduleName: string]: string[] }>; | ||
interface BannedImports { | ||
[moduleName: string]: true | Set<string>; | ||
} | ||
|
||
// Merge/normalize options. | ||
// E.g., ["a", { "b": ["c"], "d": ["e"] }, "f", { "f": ["g"] }] | ||
// becomes { "a": true, "b": Set(["c"]), "d": Set(["e"]), "f": true }. | ||
const bannedImports = (ctx.options as Options).reduce<BannedImports>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it's probably worth for perf reason to move options normalization to the |
||
(acc, it) => { | ||
if (typeof it === "string") { | ||
acc[it] = true; | ||
} else { | ||
Object.keys(it).forEach((moduleName) => { | ||
if (acc[moduleName] instanceof Set) { | ||
it[moduleName].forEach((bannedImport) => { | ||
(acc[moduleName] as Set<string>).add(bannedImport); | ||
}); | ||
} else if (acc[moduleName] !== true) { | ||
acc[moduleName] = new Set(it[moduleName]); | ||
} | ||
}); | ||
} | ||
return acc; | ||
}, | ||
Object.create(null) as BannedImports, | ||
); | ||
|
||
for (const name of findImports(ctx.sourceFile, ImportKind.All)) { | ||
if (ctx.options.indexOf(name.text) !== -1) { | ||
ctx.addFailure(name.getStart(ctx.sourceFile) + 1, name.end - 1, Rule.FAILURE_STRING); | ||
// TODO #3963: Resolve/normalize relative file imports to a canonical path? | ||
const importedModule = name.text; | ||
const bansForModule = bannedImports[importedModule]; | ||
|
||
// Check if at least some imports from this module are banned. | ||
if (bansForModule !== undefined) { | ||
// If importing this module is totally banned, we can error now, | ||
// without determining whether the user is importing the whole | ||
// module or named exports. | ||
if (bansForModule === true) { | ||
ctx.addFailure( | ||
name.getStart(ctx.sourceFile) + 1, | ||
name.end - 1, | ||
Rule.WHOLE_MODULE_FAILURE_STRING, | ||
); | ||
continue; | ||
} | ||
|
||
// Otherwise, find the named imports, if any, and fail if the | ||
// user tried to import any of them. We don't have named imports | ||
// when the user is importing the whole module, which includes: | ||
// | ||
// - ImportKind.Require (i.e., `require('module-specifier')`), | ||
// - ImportKind.DynamicImport (i.e., `import("module-specifier")`), | ||
// - ImportKind.ImportEquals (i.e., `import x = require()`), | ||
// - and ImportKind.ImportDeclaration, where there's a full namespace | ||
// import (i.e. `import * as x from "module-specifier"`) | ||
// | ||
// However, namedImports will be an array when we have one of the | ||
// various permutations of `import x, { a, b as c } from "y"`. | ||
// | ||
// We treat re-exports from other modules the same as attempting to | ||
// import the re-exported binding(s), as the re-export is essentially | ||
// an import followed by an export, and not treating these as an | ||
// import would allow backdoor imports of the banned bindings. So, | ||
// our last case is `ImportKind.ExportFrom`, and for that: | ||
// | ||
// - `export nameForDefault from "module"` isn't part of the ESM | ||
// syntax (yet), so we only have to handle two cases below: | ||
// `export { x } from "y"` and `export * from "specifier"`. | ||
const parentNode = name.parent; | ||
|
||
// Disable strict-boolean-expressions for the next few lines so our && | ||
// checks can help type inference figure out if when don't have undefined. | ||
// tslint:disable strict-boolean-expressions | ||
|
||
const importClause = parentNode && isImportDeclaration(parentNode) | ||
? parentNode.importClause | ||
: undefined; | ||
|
||
const importsDefaultExport = importClause && Boolean(importClause.name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether there are any benefits of doing |
||
|
||
// Below, check isNamedImports to rule out the | ||
// `import * as ns from "..."` case. | ||
const importsSpecificNamedExports = importClause | ||
&& importClause.namedBindings | ||
&& isNamedImports(importClause.namedBindings); | ||
|
||
// If parentNode is an ExportDeclaration, it must represent an | ||
// `export from`, as findImports verifies that. Then, if exportClause | ||
// is undefined, we're dealing with `export * from ...`. | ||
const reExportsSpecificNamedExports = parentNode | ||
&& isExportDeclaration(parentNode) | ||
&& Boolean(parentNode.exportClause); | ||
|
||
// tslint:enable strict-boolean-expressions | ||
|
||
if (importsDefaultExport || importsSpecificNamedExports || reExportsSpecificNamedExports) { | ||
// Add an import for the default import and any named bindings. | ||
// For the named bindings, we use the name of the export from the | ||
// module (i.e., .propertyName) over its alias in the import when | ||
// the two diverge. | ||
const toExportName = (it: ts.ImportSpecifier | ts.ExportSpecifier) => | ||
(it.propertyName || it.name).text; // tslint:disable-line strict-boolean-expressions | ||
|
||
const exportClause = reExportsSpecificNamedExports | ||
? (parentNode as ts.ExportDeclaration).exportClause! | ||
: undefined; | ||
|
||
const namedImportsOrReExports = [ | ||
...(importsDefaultExport ? ["default"] : []), | ||
...(importsSpecificNamedExports | ||
? (importClause!.namedBindings as ts.NamedImports).elements.map(toExportName) | ||
: []), | ||
...(exportClause !== undefined | ||
? exportClause.elements.map(toExportName) | ||
: []), | ||
]; | ||
|
||
for (const importName of namedImportsOrReExports) { | ||
if (bansForModule.has(importName)) { | ||
ctx.addFailureAtNode( | ||
exportClause !== undefined ? exportClause : importClause!, | ||
Rule.MAKE_NAMED_IMPORT_FAILURE_STRING(importName), | ||
); | ||
} | ||
} | ||
} else { | ||
// If we're here, the user tried to import/re-export the whole module | ||
ctx.addFailure( | ||
name.getStart(ctx.sourceFile) + 1, | ||
name.end - 1, | ||
Rule.IMPLICIT_NAMED_IMPORT_FAILURE_STRING, | ||
); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,46 @@ import { Observable } from 'rxjs'; | |
~~~~ [0] | ||
import { Observable } from 'rxjs/Observable'; | ||
|
||
import forOwn = require('lodash'); | ||
~~~~~~ [0] | ||
import forOwn = require('lodash/forOwn'); | ||
import rxjs = require('rxjs'); | ||
~~~~ [0] | ||
import Observable = require('rxjs/Observable'); | ||
|
||
import * as rxjs from "rxjs"; | ||
~~~~ [0] | ||
|
||
// non-static imports cannot be checked | ||
import {Observable} from rxjs; | ||
import forOwn = require(lodash); | ||
|
||
import * as notBlacklisted from "not-blacklisted"; | ||
|
||
export * from 'lodash'; | ||
~~~~~~ [0] | ||
export * from 'rxjs'; | ||
~~~~ [0] | ||
|
||
export { default } from "dummy"; | ||
~~~~~~~~~~~ [3] | ||
|
||
export * from "lodash"; | ||
~~~~~~ [1] | ||
|
||
import * as lodash from "lodash"; | ||
~~~~~~ [1] | ||
import { difference } from "lodash"; | ||
|
||
import { pull, difference } from "lodash"; | ||
~~~~~~~~~~~~~~~~~~~~ [2] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to not underline There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume so, but I really don't think it's worth it — the error message already specifies which is the problematic import. |
||
|
||
import lodash, { pull } from "lodash"; | ||
~~~~~~~~~~~~~~~~ [2] | ||
|
||
import { pull as notPull } from "lodash"; | ||
~~~~~~~~~~~~~~~~~~~ [2] | ||
|
||
import test from "dummy"; | ||
~~~~ [3] | ||
|
||
|
||
[0]: This import is blacklisted, import a submodule instead | ||
[0]: Importing this module is blacklisted. Try importing a submodule instead. | ||
[1]: Some named exports from this module are blacklisted for importing (or re-exporting). Import/re-export only the specific values you want, instead of the whole module. | ||
[2]: The export "pull" is blacklisted. | ||
[3]: Importing (or re-exporting) the default export is blacklisted. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"rules": { | ||
"import-blacklist": [true, "lodash", "rxjs"] | ||
"import-blacklist": [true, "rxjs", { "lodash": ["pullAll", "pull"] }, { "dummy": ["default"] }] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move
Options
out fromwalk
function and correctly typeoptions
key without casting on line 99.