Update: `no-restricted-{imports,modules}`: add “patterns” (fixes #6963) #7433

ljharb · 2016-10-23T09:13:09Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?
no-restricted-imports and no-restricted-modules

Does this change cause the rule to produce more or fewer warnings?
More, only when the "patterns" option is set.

How will the change be implemented? (New option, new default behavior, etc.)?
New schema - the existing schema of an array of strings will continue to work; an alternative object schema with "paths" will also be accepted. A "patterns" property can be added to this object to opt in to the new behavior.

Please provide some example code that this change will affect:

import pick from 'lodash/pick';
/* or */
var pick = require('lodash/pick');

What does the rule currently do for this code?
Does not warn unless the exact full path is provided.

What will the rule do after it's changed?
Allow a pattern to be provided such that a partial path may be provided.

Fixes #6963.

…nt#6963)

mention-bot · 2016-10-23T09:13:10Z

@ljharb, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @mysticatea and @makepanic to be potential reviewers.

eslintbot · 2016-10-23T09:13:11Z

LGTM

platinumazure

Besides the inline changes, could you update the docs to specify that we're using gitignore-style patterns here (and bonus if you can link to useful documentation on the supported syntax)?

Also, for no-restricted-imports, should a named import also be matched against a pattern (e.g., foo/bar pattern should match statement import bar from "foo"?).

platinumazure · 2016-10-23T13:12:26Z

docs/rules/no-restricted-imports.md

+```js
+/*eslint no-restricted-imports: ["error", { "patterns": ["lodash/*"] }]*/
+
+import pick from 'lodash/pick ';


Trailing space in the module path, presumably a typo?

whoops, yes, i'll remove that

platinumazure · 2016-10-23T13:16:02Z

lib/rules/no-restricted-imports.js

                        context.report(node, "'{{importName}}' import is restricted from being used.", {
-                            importName: value
+                            importName


While you're in here, can we switch this rule to use new-style reporting? That takes a single object argument and looks like this:

context.report({ node, message: MESSAGE, data: { importName } });

platinumazure · 2016-10-23T13:16:21Z

lib/rules/no-restricted-imports.js

+                            node,
+                            "'{{importName}}' import is restricted from being used by a pattern.",
+                            { importName }
+                        );


Could this use new-style reporting as well?

platinumazure · 2016-10-23T13:18:37Z

lib/rules/no-restricted-modules.js

+                        if (restrictedPaths.has(moduleName)) {
+                            context.report(node, "'{{moduleName}}' module is restricted from being used.", {
+                                moduleName
+                            });


Using new-style reporting here would be awesome as well!

platinumazure · 2016-10-23T13:18:56Z

lib/rules/no-restricted-modules.js

+                                node,
+                                "'{{moduleName}}' module is restricted from being used by a pattern.",
+                                { moduleName }
+                            );


Another opportunity for new-style reporting.

ljharb · 2016-10-23T17:06:59Z

@platinumazure no, import from 'foo/bar' is not the same as import { bar } from 'foo' in any way, unless the package author has gone out of their way to make it so. If you want to forbid specific named imports from a specific path or pattern (which I hadn't thought of), then I think perhaps a better schema is one that is always a single array of items - and an item can be a string path, { path: "some string" }, { pattern: "some pattern/*" }, and either of the object forms could then (now or in a followup PR) add something to handle named imports. The current schema doesn't really fit well with adding per-path named import blocking. Thoughts?

Edit: another alternative is to keep the current single-object schema, but allow each of the strings to alternatively be one of the objects in question - a bit messy, but that is a path from the current PR's schema to named-import-blocking-per-path.

eslintbot · 2016-10-23T18:07:12Z

LGTM

eslintbot · 2016-10-23T18:28:48Z

LGTM

platinumazure · 2016-10-25T18:27:58Z

If the rule hadn't been handling named imports before now, I don't think it
needs to with this PR. I just wanted clarification, which you have
provided. Thanks!

On Oct 23, 2016 12:07 PM, "Jordan Harband" notifications@github.com wrote:

@platinumazure https://github.com/platinumazure no, import from
'foo/bar' is not the same as import { bar } from 'foo' in any way, unless
the package author has gone out of their way to make it so. If you want to
forbid specific named imports from a specific path or pattern (which I
hadn't thought of), then I think perhaps a better schema is one that is
always a single array of items - and an item can be a string path, {
path: "some string" }, { pattern: "some pattern/*" }, and either of the
object forms could then (now or in a followup PR) add something to handle
named imports. The current schema doesn't really fit well with adding
per-path named import blocking. Thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7433 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeuDvN-JmL8mvUWgmIpkLKP6YBpXXks5q25Q0gaJpZM4KeFen
.

platinumazure · 2016-10-27T16:35:16Z

I'll champion this. We need one more 👍 from the team (someone who isn't vitorbal or not-an-aardvark) before this can be accepted and (hopefully) merged.

platinumazure

@ljharb I don't see anything about what sort of patterns are accepted (i.e., that they're gitignore-like patterns). Can that be added to documentation somewhere? Doesn't have to be in-depth at all. And maybe a few examples (and test cases?) about ** could be cool too?

eslintbot · 2016-10-31T07:19:50Z

LGTM

ljharb · 2016-10-31T07:20:06Z

@platinumazure updated! I added mentions and examples to both readmes, as well as tests to both rules.

nzakas · 2016-10-31T20:20:23Z

Just a heads up: we have moved to a new CLA checker on pull requests. Even if you've previously signed our CLA, we will need to you sign the new one. To do so, look at the status checks for licence/cla and click the "Details" link. Sorry for the inconvenience.

ljharb · 2016-11-04T00:15:31Z

@platinumazure ping! :-)

ljharb · 2016-11-04T00:34:08Z

Thanks everyone! Please let me know what else I should do to get this merged :-)

eslintbot · 2016-11-04T23:12:12Z

LGTM

Update: no-restricted-{imports,modules}: add “patterns” (fixes esli…

3748523

…nt#6963)

jquerybot added the CLA: Valid label Oct 23, 2016

ljharb mentioned this pull request Oct 23, 2016

Allow no-restricted-imports to restrict sub-paths of imports #6963

Closed

platinumazure requested changes Oct 23, 2016

View reviewed changes

ljharb force-pushed the no_restricted_partial_paths branch from e5b6f52 to a7d24e5 Compare October 23, 2016 18:07

ljharb force-pushed the no_restricted_partial_paths branch from a7d24e5 to c616828 Compare October 23, 2016 18:28

ilyavolodin added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 23, 2016

not-an-aardvark approved these changes Oct 26, 2016

View reviewed changes

platinumazure approved these changes Oct 26, 2016

View reviewed changes

platinumazure self-assigned this Oct 27, 2016

ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 27, 2016

vitorbal approved these changes Oct 27, 2016

View reviewed changes

platinumazure requested changes Oct 27, 2016

View reviewed changes

ljharb force-pushed the no_restricted_partial_paths branch from c616828 to 6932e47 Compare October 31, 2016 07:19

nzakas removed the CLA: Valid label Oct 31, 2016

platinumazure approved these changes Nov 4, 2016

View reviewed changes

kaicataldo mentioned this pull request Nov 4, 2016

Add the "lean bundle" use-case no-restricted-imports #7532

Closed

ljharb force-pushed the no_restricted_partial_paths branch from 6932e47 to 3748523 Compare November 4, 2016 23:12

ilyavolodin merged commit 3c379ff into eslint:master Nov 5, 2016

ljharb deleted the no_restricted_partial_paths branch November 5, 2016 03:32

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: `no-restricted-{imports,modules}`: add “patterns” (fixes #6963) #7433

Update: `no-restricted-{imports,modules}`: add “patterns” (fixes #6963) #7433

ljharb commented Oct 23, 2016 •

edited

mention-bot commented Oct 23, 2016

eslintbot commented Oct 23, 2016

platinumazure left a comment

platinumazure Oct 23, 2016

ljharb Oct 23, 2016

platinumazure Oct 23, 2016

ljharb Oct 23, 2016

platinumazure Oct 23, 2016

platinumazure Oct 23, 2016

platinumazure Oct 23, 2016

ljharb commented Oct 23, 2016 •

edited

eslintbot commented Oct 23, 2016

eslintbot commented Oct 23, 2016

platinumazure commented Oct 25, 2016

platinumazure commented Oct 27, 2016

platinumazure left a comment •

edited

eslintbot commented Oct 31, 2016

ljharb commented Oct 31, 2016

nzakas commented Oct 31, 2016

ljharb commented Nov 4, 2016

ljharb commented Nov 4, 2016

eslintbot commented Nov 4, 2016

Update: no-restricted-{imports,modules}: add “patterns” (fixes #6963) #7433

Update: no-restricted-{imports,modules}: add “patterns” (fixes #6963) #7433

Conversation

ljharb commented Oct 23, 2016 • edited

mention-bot commented Oct 23, 2016

eslintbot commented Oct 23, 2016

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure Oct 23, 2016

Choose a reason for hiding this comment

ljharb Oct 23, 2016

Choose a reason for hiding this comment

platinumazure Oct 23, 2016

Choose a reason for hiding this comment

ljharb Oct 23, 2016

Choose a reason for hiding this comment

platinumazure Oct 23, 2016

Choose a reason for hiding this comment

platinumazure Oct 23, 2016

Choose a reason for hiding this comment

platinumazure Oct 23, 2016

Choose a reason for hiding this comment

ljharb commented Oct 23, 2016 • edited

eslintbot commented Oct 23, 2016

eslintbot commented Oct 23, 2016

platinumazure commented Oct 25, 2016

platinumazure commented Oct 27, 2016

platinumazure left a comment • edited

Choose a reason for hiding this comment

eslintbot commented Oct 31, 2016

ljharb commented Oct 31, 2016

nzakas commented Oct 31, 2016

ljharb commented Nov 4, 2016

ljharb commented Nov 4, 2016

eslintbot commented Nov 4, 2016

Update: `no-restricted-{imports,modules}`: add “patterns” (fixes #6963) #7433

Update: `no-restricted-{imports,modules}`: add “patterns” (fixes #6963) #7433

ljharb commented Oct 23, 2016 •

edited

ljharb commented Oct 23, 2016 •

edited

platinumazure left a comment •

edited