Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New] no-internal-modules: Add forbid option #1846

Merged
merged 1 commit into from
Jan 31, 2021
Merged

[New] no-internal-modules: Add forbid option #1846

merged 1 commit into from
Jan 31, 2021

Conversation

guillaumewuip
Copy link
Contributor

Here is the pull request that implements #1842.

Without forbid option provided, the behavior stay the same. It should be a semver-minor PR.

With forbid option provided, the rule check if a given path is explicitly forbidden before testing if it's allowed.

@@ -14,6 +14,13 @@ ruleTester.run('no-internal-modules', rule, {
filename: testFilePath('./internal-modules/plugins/plugin.js'),
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've added tests, but maybe not enough. Don't hesitate to ask me to implement other tests if you see something I've missed :)

@coveralls
Copy link

coveralls commented Jul 4, 2020

Coverage Status

Coverage increased (+0.009%) to 97.879% when pulling 1c6a7ca on guillaumewuip:add-forbid-option-no-internal-modules into fd4b16b on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 97.851% when pulling 345cfb0 on guillaumewuip:add-forbid-option-no-internal-modules into 843055c on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'd saw this is worth a lot more test cases :-) I don't have any concrete ones in mind, but the more there are, the more convincing the PR is.

@@ -4,7 +4,10 @@ Use this rule to prevent importing the submodules of other modules.

## Rule Details

This rule has one option, `allow` which is an array of [minimatch/glob patterns](https://github.com/isaacs/node-glob#glob-primer) patterns that whitelist paths and import statements that can be imported with reaching.
This rule has two options that are arrays of [minimatch/glob patterns](https://github.com/isaacs/node-glob#glob-primer) patterns:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This rule has two options that are arrays of [minimatch/glob patterns](https://github.com/isaacs/node-glob#glob-primer) patterns:
This rule has two mutually exclusive options that are arrays of [minimatch/glob](https://github.com/isaacs/node-glob#glob-primer) patterns:

Comment on lines 41 to 42
"allow": [ "**/actions/*", "source-map-support/*" ],
"forbid": [ "**/actions/special/*" ],
Copy link
Member

Choose a reason for hiding this comment

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

should it be possible to both allow and forbid specific things? When "allow" is specified, it takes precedence over a default "forbid" of "/*". When "forbid" is specified, it takes precedence over a default "allow" of "/*".

I'd expect the schema to only allow one or the other, never both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By

I'd expect the schema to only allow one or the other, never both.

You mean the user will either provide allow array or forbid array but never both ?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. If both are provided, we have to define a precedence between them - with allow of A and forbid of B, what's intended with C? Is it allowed, because it's not forbidden? Or is it forbidden, because it's not allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I understand now! I will rework the PR to do that 👍

@guillaumewuip
Copy link
Contributor Author

@ljharb I took the time to work again on this PR. allow and forbid are now mutually exclusive. Same default behavior, should not be a breaking

@guillaumewuip
Copy link
Contributor Author

Hello @ljharb, do you need something else on this PR ?

@ljharb ljharb changed the title [New] Add forbid options for no-internal-modules [New] no-internal-modules: Add forbid option Jan 30, 2021
@ljharb ljharb merged commit 1c6a7ca into import-js:master Jan 31, 2021
@guillaumewuip guillaumewuip deleted the add-forbid-option-no-internal-modules branch January 31, 2021 12:46
@jenskuhrjorgensen
Copy link

Hi @ljharb

This feature seems to have been released in the docs, but it is not released in the newest release (v.2.22.1) which is pretty confusing. Is this a mistake? I was trying to use the feature as described in the docs, but it failed with the following error:

Error: .eslintrc:
	Configuration for rule "import/no-internal-modules" is invalid:
	Value {"forbid":["**/actions/*","source-map-support/*"]} should NOT have additional properties.

Best regards
Jens

@ljharb
Copy link
Member

ljharb commented Feb 10, 2021

It's not a mistake at all. If you're looking at the docs on master, you'll see unreleased things.

I created the "github release" for v2.22.1 yesterday, but it was actually released in September (see the changelog link). This PR has not yet been released.

@jenskuhrjorgensen
Copy link

Okay, thanks for clarifying. Never realized that I actually have to look in the release zip for the current documentation :)

@ljharb
Copy link
Member

ljharb commented Feb 11, 2021

Yes, on every project on github, you have to switch to the latest release tag to see the current documentation.

@richard-lopes-ifood
Copy link

richard-lopes-ifood commented Mar 5, 2021

When does it goes to an official release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants