import-blacklist: support banning the import of specific named exports #3926

ethanresnick · 2018-05-23T05:10:03Z

PR checklist

Addresses an existing issue: import-blacklist: allow package name preferences? #3843
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

This extends the existing functionality of import-blacklist (without any breaking changes) to make it possible to blacklist imports of specific named exports from a module. The idea is that a module might have some exports that you don't want people to use in your project (even though they're not @deprecated or @internal to the module). For example, lodash has a pull method that removes items from an array, mutating the array in place; you may want to ban importing this method, to instead require that people use difference, which is like pull except that it doesn't mutate its argument.

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[new-rule-option] [enhancement] import-blacklist: support blacklisting specific named exports

giladgray

this is very cool!

giladgray · 2018-06-01T16:52:29Z

src/rules/importBlacklistRule.ts

+ }
+ return acc;
+ },
+ {} as BannedImports, // tslint:disable-line no-object-literal-type-assertion


.reduce<BannedImports>(...) should allow you to remove this disable.

src/rules/importBlacklistRule.ts

ethanresnick · 2018-06-01T19:18:03Z

@giladgray Thanks for the review! I've responded to your comments inline.

src/rules/importBlacklistRule.ts

suchanlee · 2018-06-19T22:11:44Z

test/rules/import-blacklist/test.ts.lint

+import { difference } from "lodash";
+
+import { pull, difference } from "lodash";
+ ~~~~~~~~~~~~~~~~~~~~ [2]


is there a way to not underline difference?

I assume so, but I really don't think it's worth it — the error message already specifies which is the problematic import.

ethanresnick · 2018-06-28T04:45:16Z

@suchanlee @giladgray Is this good to merge?

suchanlee · 2018-06-28T05:37:13Z

can you merge rebase off of latest master to fix the build?

ethanresnick · 2018-06-28T06:46:49Z

@suchanlee Done!

desfero · 2018-09-15T15:36:12Z

Any updates on enhancement?

desfero · 2018-09-15T16:20:33Z

src/rules/importBlacklistRule.ts

@@ -54,9 +88,140 @@ export class Rule extends Lint.Rules.AbstractRule {
 }

 function walk(ctx: Lint.WalkContext<string[]>) {
+ type Options = Array<string | { [moduleName: string]: string[] }>;


You can move Options out from walk function and correctly type options key without casting on line 99.

function walk(ctx: Lint.WalkCONTEXT< Array<string | { [moduleName: string]: string[] }>>) {

desfero · 2018-09-15T16:30:00Z

src/rules/importBlacklistRule.ts

+ ? parentNode.importClause
+ : undefined;
+
+ const importsDefaultExport = importClause && Boolean(importClause.name);


I'm wondering whether there are any benefits of doing Boolean(importClause.name) over simple importClause.name here and on line 183 if strict-boolean-expressions is disabled?

desfero · 2018-09-16T08:05:49Z

@ethanbond @suchanlee @giladgray maybe it's worth to add support for custom message why import was blocked (have a look please at similar PR to add support for blocking only specific named exports)

desfero · 2018-09-16T08:00:09Z

src/rules/importBlacklistRule.ts

+ // 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>(


Actually, it's probably worth for perf reason to move options normalization to the Rule class above as it's not worth to preprocess options for each file separately.

ethanresnick · 2018-09-16T21:32:56Z

@desfero Thanks for the feedback!

You can move Options out from walk function and correctly type options key without casting on line 99.

Yup, that's nicer. I've pushed a fix for this.

I'm wondering whether there are any benefits of doing Boolean(importClause.name) over simple importClause.name here and on line 183 if strict-boolean-expressions is disabled?

I don't know, but I'd rather not disable lint rules if possible. strict-boolean-expressions is sort of a stylistic thing, imo, and if the TSLint team wants it enabled to make sure the variables actually hold a boolean, then I'm fine doing the casting.

maybe it's worth to add support for custom message why import was blocked

I really like this idea. Hopefully, we can get this merged first, though, and then add it. I think it would be as simple as allowing a [name, message] tuple or { name, message } object anywhere that a string is currently allowed in the options. That way, messages could be associated with specific named imports as well as whole modules:

[
  true, 
  "rxjs", 
  { "name": "lodash", "message": "use underscore" },
  { "underscore": [{ "name": "pick", "message": "use x instead" }, "y"] }
]

Actually, it's probably worth for perf reason to move options normalization to the Rule class above as it's not worth to preprocess options for each file separately.

Makes sense. I'm not sure exactly how the initialization of Rule classes happens, though, or whether it's kosher to add a constructor (which is presumably where I'd compute this), as none of the other Rules seem to define one. Open to input here from the TSLint team.

JoshuaKGoldberg

Rule messages could only mention importing or re-exporting as needed, but not a blocker for merging. ✔

ethanresnick changed the title ~~import-blacklist: support banning specific named imports~~ import-blacklist: support banning the import of specific named exports May 23, 2018

ethanresnick force-pushed the ban-specific-imports branch from 1b18d12 to 4fc62ce Compare May 23, 2018 05:33

giladgray reviewed Jun 1, 2018

View reviewed changes

ethanresnick force-pushed the ban-specific-imports branch from 1e157ec to f8a41a8 Compare June 1, 2018 19:39

ethanresnick mentioned this pull request Jun 3, 2018

import-blacklist: allow package name preferences? #3843

Closed

suchanlee reviewed Jun 19, 2018

View reviewed changes

ethanresnick force-pushed the ban-specific-imports branch from b98e620 to d4a390e Compare June 20, 2018 19:37

ethanresnick added 2 commits June 28, 2018 02:40

Merge remote-tracking branch 'upstream/master' into ban-specific-imports

d18175c

import-blacklist: support banning specific named imports

692c0aa

ethanresnick force-pushed the ban-specific-imports branch from d4a390e to 692c0aa Compare June 28, 2018 06:44

JoshuaKGoldberg mentioned this pull request Jul 5, 2018

Rule request: preferred-package-imports microsoft/tslint-microsoft-contrib#422

Closed

desfero reviewed Sep 15, 2018

View reviewed changes

desfero suggested changes Sep 16, 2018

View reviewed changes

import-blacklist: small cleanup

c9581f7

Merge remote-tracking branch 'palantir/master' into ban-specific-imports

3c3eb40

JoshuaKGoldberg approved these changes Nov 7, 2018

View reviewed changes

JoshuaKGoldberg added the PR: waiting for reviewer label Nov 7, 2018

desfero approved these changes Nov 21, 2018

View reviewed changes

ericanderson merged commit 09bc892 into palantir:master Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import-blacklist: support banning the import of specific named exports #3926

import-blacklist: support banning the import of specific named exports #3926

ethanresnick commented May 23, 2018 •

edited by giladgray

Loading

giladgray left a comment

giladgray Jun 1, 2018

ethanresnick Jun 1, 2018

ethanresnick commented Jun 1, 2018

suchanlee Jun 19, 2018

ethanresnick Jun 20, 2018

ethanresnick commented Jun 28, 2018

suchanlee commented Jun 28, 2018

ethanresnick commented Jun 28, 2018

desfero commented Sep 15, 2018

desfero Sep 15, 2018

desfero Sep 15, 2018

desfero commented Sep 16, 2018 •

edited

Loading

desfero Sep 16, 2018

ethanresnick commented Sep 16, 2018 •

edited

Loading

JoshuaKGoldberg left a comment

import-blacklist: support banning the import of specific named exports #3926

import-blacklist: support banning the import of specific named exports #3926

Conversation

ethanresnick commented May 23, 2018 • edited by giladgray Loading

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

giladgray left a comment

Choose a reason for hiding this comment

giladgray Jun 1, 2018

Choose a reason for hiding this comment

ethanresnick Jun 1, 2018

Choose a reason for hiding this comment

ethanresnick commented Jun 1, 2018

suchanlee Jun 19, 2018

Choose a reason for hiding this comment

ethanresnick Jun 20, 2018

Choose a reason for hiding this comment

ethanresnick commented Jun 28, 2018

suchanlee commented Jun 28, 2018

ethanresnick commented Jun 28, 2018

desfero commented Sep 15, 2018

desfero Sep 15, 2018

Choose a reason for hiding this comment

desfero Sep 15, 2018

Choose a reason for hiding this comment

desfero commented Sep 16, 2018 • edited Loading

desfero Sep 16, 2018

Choose a reason for hiding this comment

ethanresnick commented Sep 16, 2018 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ethanresnick commented May 23, 2018 •

edited by giladgray

Loading

desfero commented Sep 16, 2018 •

edited

Loading

ethanresnick commented Sep 16, 2018 •

edited

Loading