Skip to content

Commit

Permalink
[New] no-internal-modules: Add forbid option
Browse files Browse the repository at this point in the history 
Fixes #1842
  • Loading branch information
@guillaumewuip @ljharb
guillaumewuip authored and ljharb committed Jul 17, 2020
1 parent fd4b16b commit 1c6a7ca
Show file tree
Hide file tree
Showing 4 changed files with 278 additions and 19 deletions.
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Expand Up @@ -8,6 +8,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

### Added
- [`no-commonjs`]: Also detect require calls with expressionless template literals: ``` require(`x`) ``` ([#1958], thanks [@FloEdelmann])
- [`no-internal-modules`]: Add `forbid` option ([#1846], thanks [@guillaumewuip])

### Fixed
- [`export`]/TypeScript: properly detect export specifiers as children of a TS module block ([#1889], thanks [@andreubotella])
Expand Down Expand Up @@ -765,6 +766,7 @@ for info on changes for earlier releases.
[#1878]: https://github.com/benmosher/eslint-plugin-import/pull/1878
[#1854]: https://github.com/benmosher/eslint-plugin-import/issues/1854
[#1848]: https://github.com/benmosher/eslint-plugin-import/pull/1848
[#1846]: https://github.com/benmosher/eslint-plugin-import/pull/1846
[#1841]: https://github.com/benmosher/eslint-plugin-import/issues/1841
[#1836]: https://github.com/benmosher/eslint-plugin-import/pull/1836
[#1835]: https://github.com/benmosher/eslint-plugin-import/pull/1835
Expand Down Expand Up @@ -1321,4 +1323,5 @@ for info on changes for earlier releases.
[@MatthiasKunnen]: https://github.com/MatthiasKunnen
[@paztis]: https://github.com/paztis
[@FloEdelmann]: https://github.com/FloEdelmann
[@bicstone]: https://github.com/bicstone
[@bicstone]: https://github.com/bicstone
[@guillaumewuip]: https://github.com/guillaumewuip
66 changes: 64 additions & 2 deletions docs/rules/no-internal-modules.md
View file
Expand Up @@ -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 mutally exclusive options that are arrays of [minimatch/glob patterns](https://github.com/isaacs/node-glob#glob-primer) patterns:

- `allow` that include paths and import statements that can be imported with reaching.
- `forbid` that exclude paths and import statements that can be imported with reaching.

### Examples

Expand Down Expand Up @@ -33,7 +36,7 @@ And the .eslintrc file:
...
"rules": {
"import/no-internal-modules": [ "error", {
"allow": [ "**/actions/*", "source-map-support/*" ]
"allow": [ "**/actions/*", "source-map-support/*" ],
} ]
}
}
Expand Down Expand Up @@ -68,3 +71,62 @@ import getUser from '../actions/getUser';
export * from 'source-map-support/register';
export { settings } from '../app';
```

Given the following folder structure:

```
my-project
├── actions
│ └── getUser.js
│ └── updateUser.js
├── reducer
│ └── index.js
│ └── user.js
├── redux
│ └── index.js
│ └── configureStore.js
└── app
│ └── index.js
│ └── settings.js
└── entry.js
```

And the .eslintrc file:
```
{
...
"rules": {
"import/no-internal-modules": [ "error", {
"forbid": [ "**/actions/*", "source-map-support/*" ],
} ]
}
}
```

The following patterns are considered problems:

```js
/**
* in my-project/entry.js
*/

import 'source-map-support/register';
import getUser from '../actions/getUser';

export * from 'source-map-support/register';
export getUser from '../actions/getUser';
```

The following patterns are NOT considered problems:

```js
/**
* in my-project/entry.js
*/

import 'source-map-support';
import { getUser } from '../actions';

export * from 'source-map-support';
export { getUser } from '../actions';
```
78 changes: 62 additions & 16 deletions src/rules/no-internal-modules.js
View file
Expand Up @@ -14,38 +14,49 @@ module.exports = {

schema: [
{
type: 'object',
properties: {
allow: {
type: 'array',
items: {
type: 'string',
oneOf: [
{
type: 'object',
properties: {
allow: {
type: 'array',
items: {
type: 'string',
},
},
},
additionalProperties: false,
},
},
additionalProperties: false,
{
type: 'object',
properties: {
forbid: {
type: 'array',
items: {
type: 'string',
},
},
},
additionalProperties: false,
},
],
},
],
},

create: function noReachingInside(context) {
const options = context.options[0] || {};
const allowRegexps = (options.allow || []).map(p => minimatch.makeRe(p));

// test if reaching to this destination is allowed
function reachingAllowed(importPath) {
return allowRegexps.some(re => re.test(importPath));
}
const forbidRegexps = (options.forbid || []).map(p => minimatch.makeRe(p));

// minimatch patterns are expected to use / path separators, like import
// statements, so normalize paths to use the same
function normalizeSep(somePath) {
return somePath.split('\\').join('/');
}

// find a directory that is being reached into, but which shouldn't be
function isReachViolation(importPath) {
const steps = normalizeSep(importPath)
function toSteps(somePath) {
return normalizeSep(somePath)
.split('/')
.reduce((acc, step) => {
if (!step || step === '.') {
Expand All @@ -56,6 +67,20 @@ module.exports = {
return acc.concat(step);
}
}, []);
}

// test if reaching to this destination is allowed
function reachingAllowed(importPath) {
return allowRegexps.some(re => re.test(importPath));
}

// test if reaching to this destination is forbidden
function reachingForbidden(importPath) {
return forbidRegexps.some(re => re.test(importPath));
}

function isAllowViolation(importPath) {
const steps = toSteps(importPath);

const nonScopeSteps = steps.filter(step => step.indexOf('@') !== 0);
if (nonScopeSteps.length <= 1) return false;
Expand All @@ -75,6 +100,27 @@ module.exports = {
return true;
}

function isForbidViolation(importPath) {
const steps = toSteps(importPath);

// before trying to resolve, see if the raw import (with relative
// segments resolved) matches a forbidden pattern
const justSteps = steps.join('/');

if (reachingForbidden(justSteps) || reachingForbidden(`/${justSteps}`)) return true;

// if the import statement doesn't match directly, try to match the
// resolved path if the import is resolvable
const resolved = resolve(importPath, context);
if (resolved && reachingForbidden(normalizeSep(resolved))) return true;

// this import was not forbidden by the forbidden paths so it is not a violation
return false;
}

// find a directory that is being reached into, but which shouldn't be
const isReachViolation = options.forbid ? isForbidViolation : isAllowViolation;

function checkImportForReaching(importPath, node) {
const potentialViolationTypes = ['parent', 'index', 'sibling', 'external', 'internal'];
if (potentialViolationTypes.indexOf(importType(importPath, context)) !== -1 &&
Expand Down
148 changes: 148 additions & 0 deletions tests/src/rules/no-internal-modules.js
View file
Expand Up @@ -59,6 +59,34 @@ ruleTester.run('no-internal-modules', rule, {
allow: [ '**/index{.js,}' ],
} ],
}),
test({
code: 'import a from "./plugin2/thing"',
filename: testFilePath('./internal-modules/plugins/plugin.js'),
options: [ {
forbid: [ '**/api/*' ],
} ],
}),
test({
code: 'const a = require("./plugin2/thing")',
filename: testFilePath('./internal-modules/plugins/plugin.js'),
options: [ {
forbid: [ '**/api/*' ],
} ],
}),
test({
code: 'import b from "app/a"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ 'app/**/**' ],
} ],
}),
test({
code: 'import b from "@org/package"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '@org/package/*' ],
} ],
}),
// exports
test({
code: 'export {a} from "./internal.js"',
Expand Down Expand Up @@ -114,6 +142,34 @@ ruleTester.run('no-internal-modules', rule, {
parser: parser,
}),
]),
test({
code: 'export * from "./plugin2/thing"',
filename: testFilePath('./internal-modules/plugins/plugin.js'),
options: [ {
forbid: [ '**/api/*' ],
} ],
}),
test({
code: 'export * from "app/a"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ 'app/**/**' ],
} ],
}),
test({
code: 'export { b } from "@org/package"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '@org/package/*' ],
} ],
}),
test({
code: 'export * from "./app/index.js";\nexport * from "./app/index"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '**/index.ts' ],
} ],
}),
],

invalid: [
Expand Down Expand Up @@ -184,6 +240,70 @@ ruleTester.run('no-internal-modules', rule, {
},
],
}),
test({
code: 'import "./app/index.js"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '*/app/*' ],
} ],
errors: [ {
message: 'Reaching to "./app/index.js" is not allowed.',
line: 1,
column: 8,
} ],
}),
test({
code: 'import b from "@org/package"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '@org/**' ],
} ],
errors: [ {
message: 'Reaching to "@org/package" is not allowed.',
line: 1,
column: 15,
} ],
}),
test({
code: 'import b from "app/a/b"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ 'app/**/**' ],
} ],
errors: [ {
message: 'Reaching to "app/a/b" is not allowed.',
line: 1,
column: 15,
} ],
}),
test({
code: 'import get from "lodash.get"',
filename: testFilePath('./internal-modules/plugins/plugin2/index.js'),
options: [ {
forbid: [ 'lodash.*' ],
} ],
errors: [ {
message: 'Reaching to "lodash.get" is not allowed.',
line: 1,
column: 17,
} ],
}),
test({
code: 'import "./app/index.js";\nimport "./app/index"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '**/index{.js,}' ],
} ],
errors: [ {
message: 'Reaching to "./app/index.js" is not allowed.',
line: 1,
column: 8,
}, {
message: 'Reaching to "./app/index" is not allowed.',
line: 2,
column: 8,
} ],
}),
// exports
test({
code: 'export * from "./plugin2/index.js";\nexport * from "./plugin2/app/index"',
Expand Down Expand Up @@ -251,5 +371,33 @@ ruleTester.run('no-internal-modules', rule, {
},
],
}),
test({
code: 'export * from "./plugin2/thing"',
filename: testFilePath('./internal-modules/plugins/plugin.js'),
options: [ {
forbid: [ '**/plugin2/*' ],
} ],
errors: [
{
message: 'Reaching to "./plugin2/thing" is not allowed.',
line: 1,
column: 15,
},
],
}),
test({
code: 'export * from "app/a"',
filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'),
options: [ {
forbid: [ '**' ],
} ],
errors: [
{
message: 'Reaching to "app/a" is not allowed.',
line: 1,
column: 15,
},
],
}),
],
});

0 comments on commit 1c6a7ca

Please sign in to comment.