-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement new rule: no-internal-modules #485
Changes from 5 commits
f072e8a
a2dd4fd
4530ae6
f872281
d0ce3a8
0509d70
3d00542
152dd37
b158a32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# no-reaching-inside | ||
|
||
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-reaching-inside": [ "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'; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
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 seperators, 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)) return false | ||
if (reachingAllowed(`/${justSteps}`)) return false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more nitpicking |
||
|
||
// 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) return false | ||
if (reachingAllowed(normalizeSep(resolved))) return false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even more nitpicking |
||
|
||
// this import was not allowed by the allowed paths, and reaches | ||
// so it is a violation | ||
return true | ||
} | ||
|
||
function checkImportForReaching(importPath, node) { | ||
switch (importType(importPath, context)) { | ||
case 'parent': | ||
case 'index': | ||
case 'sibling': | ||
case 'external': | ||
case 'internal': { | ||
if (isReachViolation(importPath)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An other way to do this, which I prefer: const potentialViolationTypes = ['parent', 'index', 'sibling', 'external', 'internal']
function checkImportForReaching(importPath, node) {
if (potentialViolationTypes.indexOf(importType(importPath, context)) !== -1 &&
isReachViolation(importPath)
) {
// report
}
} |
||
context.report({ | ||
node, | ||
message: `Reaching to "${importPath}" is not allowed.`, | ||
}) | ||
} | ||
break | ||
} | ||
} | ||
} | ||
|
||
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, | ||
}, | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
require('babel-register') | ||
require('./tests/src/rules/no-reaching-inside.js') |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
import { RuleTester } from 'eslint' | ||
import rule from 'rules/no-reaching-inside' | ||
|
||
import { test, testFilePath } from '../utils' | ||
|
||
const ruleTester = new RuleTester() | ||
|
||
ruleTester.run('no-reaching-inside', rule, { | ||
valid: [ | ||
test({ | ||
code: 'import a from "./plugin2"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin.js'), | ||
options: [], | ||
}), | ||
test({ | ||
code: 'const a = require("./plugin2")', | ||
filename: testFilePath('./reaching-inside/plugins/plugin.js'), | ||
}), | ||
test({ | ||
code: 'const a = require("./plugin2/")', | ||
filename: testFilePath('./reaching-inside/plugins/plugin.js'), | ||
}), | ||
test({ | ||
code: 'const dynamic = "./plugin2/"; const a = require(dynamic)', | ||
filename: testFilePath('./reaching-inside/plugins/plugin.js'), | ||
}), | ||
test({ | ||
code: 'import b from "./internal.js"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin2/index.js'), | ||
}), | ||
test({ | ||
code: 'import get from "lodash.get"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin2/index.js'), | ||
}), | ||
test({ | ||
code: 'import b from "../../api/service"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin2/internal.js'), | ||
options: [ { | ||
allow: [ '**/api/*' ], | ||
} ], | ||
}), | ||
test({ | ||
code: 'import "jquery/dist/jquery"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin2/internal.js'), | ||
options: [ { | ||
allow: [ 'jquery/dist/*' ], | ||
} ], | ||
}), | ||
test({ | ||
code: 'import "./app/index.js";\nimport "./app/index"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin2/internal.js'), | ||
options: [ { | ||
allow: [ '**/index{.js,}' ], | ||
} ], | ||
}), | ||
], | ||
|
||
invalid: [ | ||
test({ | ||
code: 'import "./plugin2/index.js";\nimport "./plugin2/app/index"', | ||
filename: testFilePath('./reaching-inside/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('./reaching-inside/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('./reaching-inside/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('./reaching-inside/plugins/plugin.js'), | ||
options: [ { | ||
allow: [ '**/reaching-inside/*' ], | ||
} ], | ||
errors: [ | ||
{ | ||
message: 'Reaching to "../api/service/index" is not allowed.', | ||
line: 1, | ||
column: 15, | ||
}, | ||
], | ||
}), | ||
test({ | ||
code: 'import get from "debug/node"', | ||
filename: testFilePath('./reaching-inside/plugins/plugin.js'), | ||
errors: [ | ||
{ | ||
message: 'Reaching to "debug/node" is not allowed.', | ||
line: 1, | ||
column: 17, | ||
}, | ||
], | ||
}), | ||
], | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: seperators --> separators 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spell that word wrong everywhere, all the time, I think I need to setup autohotkey just for it.