From c57beae53f6e49778ec0dfbdf450750fb88e787b Mon Sep 17 00:00:00 2001 From: Spencer Date: Thu, 15 Sep 2016 03:12:18 -0700 Subject: [PATCH] Implement new rule: no-internal-modules (#485) * Implement new rule: no-reaching-inside * normalize path segments all path segments to '/' * point to minimatch/glob docs to define glob syntax * refactor no-reaching-inside, allow now points to importable modules/files * add test to verify that "no-reaching/allow" can limit the depth of matches * code style improvements * rename to no-internal-modules * remove extra test executor --- docs/rules/no-internal-modules.md | 64 ++++++++++ src/index.js | 1 + src/rules/no-internal-modules.js | 92 ++++++++++++++ .../internal-modules/api/service/index.js | 0 .../files/internal-modules/plugins/plugin.js | 0 .../plugins/plugin2/app/index.js | 0 .../internal-modules/plugins/plugin2/index.js | 0 .../plugins/plugin2/internal.js | 0 .../files/node_modules/jquery/dist/jquery.js | 1 + tests/files/package.json | 1 + tests/src/rules/no-internal-modules.js | 115 ++++++++++++++++++ 11 files changed, 274 insertions(+) create mode 100644 docs/rules/no-internal-modules.md create mode 100644 src/rules/no-internal-modules.js create mode 100644 tests/files/internal-modules/api/service/index.js create mode 100644 tests/files/internal-modules/plugins/plugin.js create mode 100644 tests/files/internal-modules/plugins/plugin2/app/index.js create mode 100644 tests/files/internal-modules/plugins/plugin2/index.js create mode 100644 tests/files/internal-modules/plugins/plugin2/internal.js create mode 100644 tests/files/node_modules/jquery/dist/jquery.js create mode 100644 tests/src/rules/no-internal-modules.js diff --git a/docs/rules/no-internal-modules.md b/docs/rules/no-internal-modules.md new file mode 100644 index 000000000..1d8fc56d2 --- /dev/null +++ b/docs/rules/no-internal-modules.md @@ -0,0 +1,64 @@ +# no-internal-modules + +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. + +### Examples + +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", { + "allow": [ "**/actions/*", "source-map-support/*" ] + } ] + } +} +``` + +The following patterns are considered problems: + +```js +/** + * in my-project/entry.js + */ + +import { settings } from './app/index'; // Reaching to "./app/index" is not allowed +import userReducer from './reducer/user'; // Reaching to "./reducer/user" is not allowed +import configureStore from './redux/configureStore'; // Reaching to "./redux/configureStore" is not allowed +``` + +The following patterns are NOT considered problems: + +```js +/** + * in my-project/entry.js + */ + +import 'source-map-support/register'; +import { settings } from '../app'; +import getUser from '../actions/getUser'; +``` diff --git a/src/index.js b/src/index.js index 5f6077881..406d53ee5 100644 --- a/src/index.js +++ b/src/index.js @@ -8,6 +8,7 @@ export const rules = { 'no-mutable-exports': require('./rules/no-mutable-exports'), 'extensions': require('./rules/extensions'), 'no-restricted-paths': require('./rules/no-restricted-paths'), + 'no-internal-modules': require('./rules/no-internal-modules'), 'no-named-as-default': require('./rules/no-named-as-default'), 'no-named-as-default-member': require('./rules/no-named-as-default-member'), diff --git a/src/rules/no-internal-modules.js b/src/rules/no-internal-modules.js new file mode 100644 index 000000000..85452df67 --- /dev/null +++ b/src/rules/no-internal-modules.js @@ -0,0 +1,92 @@ +import find from 'lodash.find' +import minimatch from 'minimatch' + +import resolve from '../core/resolve' +import importType from '../core/importType' +import isStaticRequire from '../core/staticRequire' + +module.exports = 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 !!find(allowRegexps, re => re.test(importPath)) + } + + // 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) + .split('/') + .reduce((acc, step) => { + if (!step || step === '.') { + return acc + } else if (step === '..') { + return acc.slice(0, -1) + } else { + return acc.concat(step) + } + }, []) + + if (steps.length <= 1) return false + + // before trying to resolve, see if the raw import (with relative + // segments resolved) matches an allowed pattern + const justSteps = steps.join('/') + if (reachingAllowed(justSteps) || reachingAllowed(`/${justSteps}`)) return false + + // 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 || reachingAllowed(normalizeSep(resolved))) return false + + // this import was not allowed by the allowed paths, and reaches + // so it is a violation + return true + } + + function checkImportForReaching(importPath, node) { + const potentialViolationTypes = ['parent', 'index', 'sibling', 'external', 'internal'] + if (potentialViolationTypes.indexOf(importType(importPath, context)) !== -1 && + isReachViolation(importPath) + ) { + context.report({ + node, + message: `Reaching to "${importPath}" is not allowed.`, + }) + } + } + + return { + ImportDeclaration(node) { + checkImportForReaching(node.source.value, node.source) + }, + CallExpression(node) { + if (isStaticRequire(node)) { + const [ firstArgument ] = node.arguments + checkImportForReaching(firstArgument.value, firstArgument) + } + }, + } +} + +module.exports.schema = [ + { + type: 'object', + properties: { + allow: { + type: 'array', + items: { + type: 'string', + }, + }, + }, + additionalProperties: false, + }, +] diff --git a/tests/files/internal-modules/api/service/index.js b/tests/files/internal-modules/api/service/index.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/internal-modules/plugins/plugin.js b/tests/files/internal-modules/plugins/plugin.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/internal-modules/plugins/plugin2/app/index.js b/tests/files/internal-modules/plugins/plugin2/app/index.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/internal-modules/plugins/plugin2/index.js b/tests/files/internal-modules/plugins/plugin2/index.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/internal-modules/plugins/plugin2/internal.js b/tests/files/internal-modules/plugins/plugin2/internal.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/node_modules/jquery/dist/jquery.js b/tests/files/node_modules/jquery/dist/jquery.js new file mode 100644 index 000000000..ea0d2c4cc --- /dev/null +++ b/tests/files/node_modules/jquery/dist/jquery.js @@ -0,0 +1 @@ +module.exports = 'jQuery' diff --git a/tests/files/package.json b/tests/files/package.json index 7aaed30f3..3be7b41ae 100644 --- a/tests/files/package.json +++ b/tests/files/package.json @@ -9,6 +9,7 @@ }, "dependencies": { "@scope/core": "^1.0.0", + "jquery": "^3.1.0", "lodash.cond": "^4.3.0", "pkg-up": "^1.0.0" }, diff --git a/tests/src/rules/no-internal-modules.js b/tests/src/rules/no-internal-modules.js new file mode 100644 index 000000000..26b164363 --- /dev/null +++ b/tests/src/rules/no-internal-modules.js @@ -0,0 +1,115 @@ +import { RuleTester } from 'eslint' +import rule from 'rules/no-internal-modules' + +import { test, testFilePath } from '../utils' + +const ruleTester = new RuleTester() + +ruleTester.run('no-internal-modules', rule, { + valid: [ + test({ + code: 'import a from "./plugin2"', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + options: [], + }), + test({ + code: 'const a = require("./plugin2")', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + }), + test({ + code: 'const a = require("./plugin2/")', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + }), + test({ + code: 'const dynamic = "./plugin2/"; const a = require(dynamic)', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + }), + test({ + code: 'import b from "./internal.js"', + filename: testFilePath('./internal-modules/plugins/plugin2/index.js'), + }), + test({ + code: 'import get from "lodash.get"', + filename: testFilePath('./internal-modules/plugins/plugin2/index.js'), + }), + test({ + code: 'import b from "../../api/service"', + filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'), + options: [ { + allow: [ '**/api/*' ], + } ], + }), + test({ + code: 'import "jquery/dist/jquery"', + filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'), + options: [ { + allow: [ 'jquery/dist/*' ], + } ], + }), + test({ + code: 'import "./app/index.js";\nimport "./app/index"', + filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'), + options: [ { + allow: [ '**/index{.js,}' ], + } ], + }), + ], + + invalid: [ + test({ + code: 'import "./plugin2/index.js";\nimport "./plugin2/app/index"', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + options: [ { + allow: [ '*/index.js' ], + } ], + errors: [ { + message: 'Reaching to "./plugin2/app/index" is not allowed.', + line: 2, + column: 8, + } ], + }), + test({ + code: 'import "./app/index.js"', + filename: testFilePath('./internal-modules/plugins/plugin2/internal.js'), + errors: [ { + message: 'Reaching to "./app/index.js" is not allowed.', + line: 1, + column: 8, + } ], + }), + test({ + code: 'import b from "./plugin2/internal"', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + errors: [ { + message: 'Reaching to "./plugin2/internal" is not allowed.', + line: 1, + column: 15, + } ], + }), + test({ + code: 'import a from "../api/service/index"', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + options: [ { + allow: [ '**/internal-modules/*' ], + } ], + errors: [ + { + message: 'Reaching to "../api/service/index" is not allowed.', + line: 1, + column: 15, + }, + ], + }), + test({ + code: 'import get from "debug/node"', + filename: testFilePath('./internal-modules/plugins/plugin.js'), + errors: [ + { + message: 'Reaching to "debug/node" is not allowed.', + line: 1, + column: 17, + }, + ], + }), + ], +})