diff --git a/CHANGELOG.md b/CHANGELOG.md index 347034bbc..ae9c0dfd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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]) @@ -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 @@ -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 \ No newline at end of file +[@bicstone]: https://github.com/bicstone +[@guillaumewuip]: https://github.com/guillaumewuip \ No newline at end of file diff --git a/docs/rules/no-internal-modules.md b/docs/rules/no-internal-modules.md index 7bbb2edd1..d957e26f3 100644 --- a/docs/rules/no-internal-modules.md +++ b/docs/rules/no-internal-modules.md @@ -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 @@ -33,7 +36,7 @@ And the .eslintrc file: ... "rules": { "import/no-internal-modules": [ "error", { - "allow": [ "**/actions/*", "source-map-support/*" ] + "allow": [ "**/actions/*", "source-map-support/*" ], } ] } } @@ -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'; +``` diff --git a/src/rules/no-internal-modules.js b/src/rules/no-internal-modules.js index d23bb36dd..a33f23b47 100644 --- a/src/rules/no-internal-modules.js +++ b/src/rules/no-internal-modules.js @@ -14,16 +14,32 @@ 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, + }, + ], }, ], }, @@ -31,11 +47,7 @@ module.exports = { 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 @@ -43,9 +55,8 @@ module.exports = { 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 === '.') { @@ -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; @@ -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 && diff --git a/tests/src/rules/no-internal-modules.js b/tests/src/rules/no-internal-modules.js index 1723f7df6..2bad32c46 100644 --- a/tests/src/rules/no-internal-modules.js +++ b/tests/src/rules/no-internal-modules.js @@ -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"', @@ -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: [ @@ -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"', @@ -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, + }, + ], + }), ], });