New rule: import-blacklist #1841
Conversation
This rule checks for require/import statements with a user-specified list of banned strings. This is useful to prevent accidental inclusions of entire libraries, when tree shaking is not possible. E.g. importing "material-ui" instead of "material-ui/lib/something".
Fix linting errors
Fix linting errors
|
Don't want to spam you @use-strict but please sign the palantir CLA in your branch to get this rolling, a review is also welcome 😄 |
|
|
@SimonSchick , done. Great job, thanks! |
|
Noticed conflict, will merge/rebase after I got approval. |
There was a problem hiding this comment.
@SimonSchick thanks for bearing with the broken build on master
| minLength: 1, | ||
| }, | ||
| optionExamples: ["true", '[true, "rxjs", "lodash"]'], | ||
| type: "typescript", |
There was a problem hiding this comment.
I don't think this is typescript-only -- I'd throw it in the "functionality" bucket (kind of a catch-all)
There was a problem hiding this comment.
Oh yea, that is a copy paste error, sorry 😛
| typescriptOnly: false, | ||
| }; | ||
|
|
||
| public static FAILURE_STRING = "This import is not allowed."; |
There was a problem hiding this comment.
Suggestion: "This import is blacklisted, consider importing a submodule instead"
Minor: we're a little inconsistent about this elsewhere, but I'd prefer to omit the trailing period in this failure string.
There was a problem hiding this comment.
I think I gonna go for a slightly more imperative and concise message, feel free to complain tho 😛
| super(sourceFile, options); | ||
| } | ||
|
|
||
| protected isListed(text: string): boolean { |
There was a problem hiding this comment.
also I think a nicer method signature would be private isModuleBlacklisted(moduleName: string): boolean
| } | ||
|
|
||
| class NoRequireFullLibraryWalker extends Lint.RuleWalker { | ||
| constructor (sourceFile: ts.SourceFile, options: Lint.IOptions, private blackList: string[]) { |
There was a problem hiding this comment.
minor: can you remove the private parameter property syntax and do this the old fashioned way, this.blacklist = blacklist? I find that more readable. (note the lack of camelCase, just "blacklist", not "blackList".)
| super.visitImportDeclaration(node); | ||
| } | ||
|
|
||
| private failure (node: ts.Node): void { |
There was a problem hiding this comment.
question: is there any more specific common type for the node parameter here that we can declare?
minor: can you please call this method reportFailure to be more verbose / readable
|
Happy? 😄 |
|
looks great, thanks @SimonSchick! |
PR checklist
What changes did you make?
Cont. of #1549
I was given permission by the author to continue his work:
I added tests.
Fixed problems detected by tests (somewhat of a refactor too).
Added license.
Also due to a previous change master broke, I fixed this in my PR to make it build for now.
Is there anything you'd like reviewers to focus on?
Perhaps a better error message? I'm not good with words.