From 5fa2851adc2d27c01d5b4ce1f4f3af10999d775b Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 19 Mar 2018 07:33:51 -0400 Subject: [PATCH 1/9] wip: no-cycle support with general dependency "imports" map in ExportMap --- src/ExportMap.js | 38 ++++++++++++++++++++------ src/index.js | 1 + src/rules/no-cycle.js | 62 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 src/rules/no-cycle.js diff --git a/src/ExportMap.js b/src/ExportMap.js index aefec2ba3..1a35d6923 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -21,7 +21,16 @@ export default class ExportMap { this.namespace = new Map() // todo: restructure to key on path, value is resolver + map of names this.reexports = new Map() - this.dependencies = new Map() + /** + * star-exports + * @type {Set} of () => ExportMap + */ + this.dependencies = new Set() + /** + * dependencies of this module that are not explicitly re-exported + * @type {Map} from path = () => ExportMap + */ + this.imports = new Map() this.errors = [] } @@ -46,7 +55,7 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies.values()) { + for (let dep of this.dependencies) { let innerMap = dep() // todo: report as unresolved? @@ -88,7 +97,7 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies.values()) { + for (let dep of this.dependencies) { let innerMap = dep() // todo: report as unresolved? if (!innerMap) continue @@ -125,7 +134,7 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies.values()) { + for (let dep of this.dependencies) { let innerMap = dep() // todo: report as unresolved? if (!innerMap) continue @@ -373,6 +382,18 @@ ExportMap.parse = function (path, content, context) { return object } + function captureDependency(declaration) { + if (declaration.source == null) return + + const p = remotePath(declaration) + if (p == null) return + if (m.imports.has(p)) return + + const getter = () => ExportMap.for(p, context) + m.imports.set(p, getter) + return getter + } + ast.body.forEach(function (n) { @@ -386,14 +407,14 @@ ExportMap.parse = function (path, content, context) { } if (n.type === 'ExportAllDeclaration') { - let remoteMap = remotePath(n) - if (remoteMap == null) return - m.dependencies.set(remoteMap, () => ExportMap.for(remoteMap, context)) + const getter = captureDependency(n) + if (getter) m.dependencies.add(getter) return } // capture namespaces in case of later export if (n.type === 'ImportDeclaration') { + captureDependency(n) let ns if (n.specifiers.some(s => s.type === 'ImportNamespaceSpecifier' && (ns = s))) { namespaces.set(ns.local.name, n) @@ -401,7 +422,8 @@ ExportMap.parse = function (path, content, context) { return } - if (n.type === 'ExportNamedDeclaration'){ + if (n.type === 'ExportNamedDeclaration') { + captureDependency(n) // capture declaration if (n.declaration != null) { switch (n.declaration.type) { diff --git a/src/index.js b/src/index.js index 6c5e11252..2d6352b83 100644 --- a/src/index.js +++ b/src/index.js @@ -12,6 +12,7 @@ export const rules = { 'group-exports': require('./rules/group-exports'), 'no-self-import': require('./rules/no-self-import'), + 'no-cycle': require('./rules/no-cycle'), 'no-named-default': require('./rules/no-named-default'), '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-cycle.js b/src/rules/no-cycle.js new file mode 100644 index 000000000..394c94e2e --- /dev/null +++ b/src/rules/no-cycle.js @@ -0,0 +1,62 @@ +/** + * @fileOverview Ensures that no imported module imports the linted module. + * @author Ben Mosher + */ + +import Exports from '../ExportMap' +import moduleVisitor, { optionsSchema } from 'eslint-module-utils/moduleVisitor' +import docsUrl from '../docsUrl' + +// todo: cache cycles / deep relationships for faster repeat evaluation +module.exports = { + meta: { + docs: { + url: docsUrl('no-cycle'), + }, + + schema: [optionsSchema], + }, + + create: function (context) { + + const myPath = context.getFilename() + + function checkSourceValue(source) { + const imported = Exports.get(source.value, context) + + if (imported === undefined) { + return // no-unresolved territory + } + + if (imported.path === myPath) { + // todo: report direct self import? + return + } + + const untraversed = [imported] + const traversed = new Set() + function detectCycle(m) { + if (traversed.has(m.path)) return + traversed.add(m.path) + + for (let [path, getter] of m.imports) { + if (path === context) return true + if (traversed.has(path)) continue + untraversed.push(getter()) + } + } + + while (untraversed.length > 0) { + if (detectCycle(untraversed.pop())) { + // todo: report + + // todo: cache + + return + } + } + } + + return moduleVisitor(checkSourceValue, context.options[0]) + }, +} From 0c21c4e0f6e8248f56561ad66dfcfa7d21ba31a1 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 19 Mar 2018 07:44:11 -0400 Subject: [PATCH 2/9] sublime-linter project tweaks --- import.sublime-project | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/import.sublime-project b/import.sublime-project index d2ff5f73a..3b7feb346 100644 --- a/import.sublime-project +++ b/import.sublime-project @@ -16,8 +16,12 @@ }, "eslint_d": { + "disable": true, "chdir": "${project}" } + }, + "paths": { + "osx": ["${project}/node_modules/.bin"] } } } From f7c48b5e819ffb2dd310bc107a987c605db9305c Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Tue, 20 Mar 2018 21:32:34 -0400 Subject: [PATCH 3/9] no-cycle: real rule! first draft, perf is likely atrocious --- src/ExportMap.js | 3 +- src/rules/no-cycle.js | 49 ++++++++++--------- tests/files/cycles/depth-one.js | 2 + tests/files/cycles/depth-three-indirect.js | 5 ++ tests/files/cycles/depth-three-star.js | 2 + tests/files/cycles/depth-two.js | 2 + tests/files/cycles/depth-zero.js | 1 + tests/src/rules/no-cycle.js | 55 ++++++++++++++++++++++ utils/moduleVisitor.js | 10 ++-- utils/parse.js | 3 ++ 10 files changed, 104 insertions(+), 28 deletions(-) create mode 100644 tests/files/cycles/depth-one.js create mode 100644 tests/files/cycles/depth-three-indirect.js create mode 100644 tests/files/cycles/depth-three-star.js create mode 100644 tests/files/cycles/depth-two.js create mode 100644 tests/files/cycles/depth-zero.js create mode 100644 tests/src/rules/no-cycle.js diff --git a/src/ExportMap.js b/src/ExportMap.js index 1a35d6923..4325de5e0 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -390,7 +390,7 @@ ExportMap.parse = function (path, content, context) { if (m.imports.has(p)) return const getter = () => ExportMap.for(p, context) - m.imports.set(p, getter) + m.imports.set(p, { getter, source: declaration.source }) return getter } @@ -423,7 +423,6 @@ ExportMap.parse = function (path, content, context) { } if (n.type === 'ExportNamedDeclaration') { - captureDependency(n) // capture declaration if (n.declaration != null) { switch (n.declaration.type) { diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 394c94e2e..c9148b7fa 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -10,48 +10,51 @@ import docsUrl from '../docsUrl' // todo: cache cycles / deep relationships for faster repeat evaluation module.exports = { meta: { - docs: { - url: docsUrl('no-cycle'), - }, - + docs: { url: docsUrl('no-cycle') }, schema: [optionsSchema], }, create: function (context) { - const myPath = context.getFilename() + if (myPath === '') return // can't cycle-check a non-file - function checkSourceValue(source) { - const imported = Exports.get(source.value, context) + function checkSourceValue(sourceNode, importer) { + const imported = Exports.get(sourceNode.value, context) - if (imported === undefined) { - return // no-unresolved territory + if (imported == null) { + return // no-unresolved territory } if (imported.path === myPath) { - // todo: report direct self import? - return + return // no-self-import territory } - const untraversed = [imported] + const untraversed = [{imported, route:[]}] const traversed = new Set() - function detectCycle(m) { + function detectCycle({imported: m, route}) { if (traversed.has(m.path)) return traversed.add(m.path) - for (let [path, getter] of m.imports) { - if (path === context) return true + for (let [path, { getter, source }] of m.imports) { + if (path === myPath) return true if (traversed.has(path)) continue - untraversed.push(getter()) + const deeper = getter() + if (deeper != null) { + untraversed.push({ + imported: deeper, + route: route.concat(source), + }) + } } } while (untraversed.length > 0) { - if (detectCycle(untraversed.pop())) { - // todo: report - - // todo: cache - + const next = untraversed.shift() // bfs! + if (detectCycle(next)) { + const message = (next.route.length > 0 + ? `Dependency cycle via ${routeString(next.route)}` + : 'Dependency cycle detected.') + context.report(importer, message) return } } @@ -60,3 +63,7 @@ module.exports = { return moduleVisitor(checkSourceValue, context.options[0]) }, } + +function routeString(route) { + return route.map(s => `${s.value}:${s.loc.start.line}`).join('=>') +} diff --git a/tests/files/cycles/depth-one.js b/tests/files/cycles/depth-one.js new file mode 100644 index 000000000..748f65f84 --- /dev/null +++ b/tests/files/cycles/depth-one.js @@ -0,0 +1,2 @@ +import foo from "./depth-zero" +export { foo } diff --git a/tests/files/cycles/depth-three-indirect.js b/tests/files/cycles/depth-three-indirect.js new file mode 100644 index 000000000..814562f44 --- /dev/null +++ b/tests/files/cycles/depth-three-indirect.js @@ -0,0 +1,5 @@ +import './depth-two' + +export function bar() { + return "side effects???" +} \ No newline at end of file diff --git a/tests/files/cycles/depth-three-star.js b/tests/files/cycles/depth-three-star.js new file mode 100644 index 000000000..3f680bcb0 --- /dev/null +++ b/tests/files/cycles/depth-three-star.js @@ -0,0 +1,2 @@ +import * as two from "./depth-two" +export { two } diff --git a/tests/files/cycles/depth-two.js b/tests/files/cycles/depth-two.js new file mode 100644 index 000000000..3f38d78a5 --- /dev/null +++ b/tests/files/cycles/depth-two.js @@ -0,0 +1,2 @@ +import { foo } from "./depth-one" +export { foo } diff --git a/tests/files/cycles/depth-zero.js b/tests/files/cycles/depth-zero.js new file mode 100644 index 000000000..c9f23e9ca --- /dev/null +++ b/tests/files/cycles/depth-zero.js @@ -0,0 +1 @@ +// export function foo() {} diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js new file mode 100644 index 000000000..b05a64100 --- /dev/null +++ b/tests/src/rules/no-cycle.js @@ -0,0 +1,55 @@ +import { test as _test, testFilePath } from '../utils' + +import { RuleTester } from 'eslint' + +const ruleTester = new RuleTester() + , rule = require('rules/no-cycle') + +const error = message => ({ ruleId: 'no-cycle', message }) + +const test = def => _test(Object.assign(def, { + filename: testFilePath("./cycles/depth-zero.js") +})) + +// describe.only("no-cycle", () => { +ruleTester.run('no-cycle', rule, { + valid: [ + // this rule doesn't care if the cycle length is 0 + test({ code: 'import foo from "./foo.js"'}), + + test({ code: 'import _ from "lodash"' }), + test({ code: 'import foo from "@scope/foo"' }), + test({ code: 'var _ = require("lodash")' }), + test({ code: 'var find = require("lodash.find")' }), + test({ code: 'var foo = require("./foo")' }), + test({ code: 'var foo = require("../foo")' }), + test({ code: 'var foo = require("foo")' }), + test({ code: 'var foo = require("./")' }), + test({ code: 'var foo = require("@scope/foo")' }), + test({ code: 'var bar = require("./bar/index")' }), + test({ code: 'var bar = require("./bar")' }), + test({ + code: 'var bar = require("./bar")', + filename: '', + }), + ], + invalid: [ + test({ + code: 'import { foo } from "./depth-one"', + errors: [error("Dependency cycle detected.")] + }), + test({ + code: 'import { foo } from "./depth-two"', + errors: [error("Dependency cycle via ./depth-one:1")] + }), + test({ + code: 'import { two } from "./depth-three-star"', + errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")] + }), + test({ + code: 'import { bar } from "./depth-three-indirect"', + errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")] + }), + ], +}) +// }) diff --git a/utils/moduleVisitor.js b/utils/moduleVisitor.js index 4248317b6..6d087a163 100644 --- a/utils/moduleVisitor.js +++ b/utils/moduleVisitor.js @@ -19,19 +19,19 @@ exports.default = function visitModules(visitor, options) { ignoreRegExps = options.ignore.map(p => new RegExp(p)) } - function checkSourceValue(source) { + function checkSourceValue(source, importer) { if (source == null) return //? // handle ignore if (ignoreRegExps.some(re => re.test(source.value))) return // fire visitor - visitor(source) + visitor(source, importer) } // for import-y declarations function checkSource(node) { - checkSourceValue(node.source) + checkSourceValue(node.source, node) } // for CommonJS `require` calls @@ -45,7 +45,7 @@ exports.default = function visitModules(visitor, options) { if (modulePath.type !== 'Literal') return if (typeof modulePath.value !== 'string') return - checkSourceValue(modulePath) + checkSourceValue(modulePath, call) } function checkAMD(call) { @@ -64,7 +64,7 @@ exports.default = function visitModules(visitor, options) { if (element.value === 'require' || element.value === 'exports') continue // magic modules: http://git.io/vByan - checkSourceValue(element) + checkSourceValue(element, call) } } diff --git a/utils/parse.js b/utils/parse.js index b921f8778..5bafdba49 100644 --- a/utils/parse.js +++ b/utils/parse.js @@ -23,6 +23,9 @@ exports.default = function parse(path, content, context) { parserOptions.comment = true parserOptions.attachComment = true + // attach node locations + parserOptions.loc = true + // provide the `filePath` like eslint itself does, in `parserOptions` // https://github.com/eslint/eslint/blob/3ec436ee/lib/linter.js#L637 parserOptions.filePath = path From 314c0b771787b8808b40d8fe82809b0c63102986 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Wed, 21 Mar 2018 21:55:43 -0400 Subject: [PATCH 4/9] fix issue (and add conspicuously absent test) with 'export *' --- src/ExportMap.js | 11 +++++------ tests/files/export-all.js | 1 + tests/src/rules/named.js | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/ExportMap.js b/src/ExportMap.js index 4325de5e0..11354a37e 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -383,15 +383,14 @@ ExportMap.parse = function (path, content, context) { } function captureDependency(declaration) { - if (declaration.source == null) return + if (declaration.source == null) return null const p = remotePath(declaration) - if (p == null) return - if (m.imports.has(p)) return + if (p == null || m.imports.has(p)) return p const getter = () => ExportMap.for(p, context) m.imports.set(p, { getter, source: declaration.source }) - return getter + return p } @@ -407,8 +406,8 @@ ExportMap.parse = function (path, content, context) { } if (n.type === 'ExportAllDeclaration') { - const getter = captureDependency(n) - if (getter) m.dependencies.add(getter) + const p = captureDependency(n) + if (p) m.dependencies.add(m.imports.get(p).getter) return } diff --git a/tests/files/export-all.js b/tests/files/export-all.js index 8839f6bb9..cfe060b7b 100644 --- a/tests/files/export-all.js +++ b/tests/files/export-all.js @@ -1 +1,2 @@ +import { foo } from './sibling-with-names' // ensure importing exported name doesn't block export * from './sibling-with-names' diff --git a/tests/src/rules/named.js b/tests/src/rules/named.js index 2364c74ed..8bd78f6eb 100644 --- a/tests/src/rules/named.js +++ b/tests/src/rules/named.js @@ -329,3 +329,18 @@ if (!CASE_SENSITIVE_FS) { ], }) } + +// export-all +ruleTester.run('named (export *)', rule, { + valid: [ + test({ + code: 'import { foo } from "./export-all"', + }), + ], + invalid: [ + test({ + code: 'import { bar } from "./export-all"', + errors: [`bar not found in './export-all'`], + }), + ], +}) From 864dbcff8e0b0f98f8093f217952ad4b45e2f9af Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Thu, 22 Mar 2018 06:14:37 -0400 Subject: [PATCH 5/9] no-cycle: explicit CJS/AMD tests (even though moduleVisitor handles it) --- tests/src/rules/no-cycle.js | 30 +++++++++++++++++++++++++----- utils/moduleVisitor.js | 4 ++-- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index b05a64100..178e9438d 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -8,7 +8,7 @@ const ruleTester = new RuleTester() const error = message => ({ ruleId: 'no-cycle', message }) const test = def => _test(Object.assign(def, { - filename: testFilePath("./cycles/depth-zero.js") + filename: testFilePath('./cycles/depth-zero.js'), })) // describe.only("no-cycle", () => { @@ -36,19 +36,39 @@ ruleTester.run('no-cycle', rule, { invalid: [ test({ code: 'import { foo } from "./depth-one"', - errors: [error("Dependency cycle detected.")] + errors: [error(`Dependency cycle detected.`)], + }), + test({ + code: 'const { foo } = require("./depth-one")', + errors: [error(`Dependency cycle detected.`)], + options: [{ commonjs: true }], + }), + test({ + code: 'require(["./depth-one"], d1 => {})', + errors: [error(`Dependency cycle detected.`)], + options: [{ amd: true }], + }), + test({ + code: 'define(["./depth-one"], d1 => {})', + errors: [error(`Dependency cycle detected.`)], + options: [{ amd: true }], }), test({ code: 'import { foo } from "./depth-two"', - errors: [error("Dependency cycle via ./depth-one:1")] + errors: [error(`Dependency cycle via ./depth-one:1`)], + }), + test({ + code: 'const { foo } = require("./depth-two")', + errors: [error(`Dependency cycle via ./depth-one:1`)], + options: [{ commonjs: true }], }), test({ code: 'import { two } from "./depth-three-star"', - errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")] + errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)], }), test({ code: 'import { bar } from "./depth-three-indirect"', - errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")] + errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)], }), ], }) diff --git a/utils/moduleVisitor.js b/utils/moduleVisitor.js index 6d087a163..2abcc8345 100644 --- a/utils/moduleVisitor.js +++ b/utils/moduleVisitor.js @@ -1,4 +1,4 @@ -"use strict" +'use strict' exports.__esModule = true /** @@ -64,7 +64,7 @@ exports.default = function visitModules(visitor, options) { if (element.value === 'require' || element.value === 'exports') continue // magic modules: http://git.io/vByan - checkSourceValue(element, call) + checkSourceValue(element, element) } } From 6933fa4e33e76218a92fc968b6e6541a19902f7d Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Sun, 25 Mar 2018 13:43:55 -0400 Subject: [PATCH 6/9] no-cycle: initial docs + maxDepth option --- docs/rules/no-cycle.md | 37 +++++++++++++++++++++++++++++++++++++ src/rules/no-cycle.js | 24 +++++++++++++++++------- utils/moduleVisitor.js | 3 --- 3 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 docs/rules/no-cycle.md diff --git a/docs/rules/no-cycle.md b/docs/rules/no-cycle.md new file mode 100644 index 000000000..2c2a8e8ef --- /dev/null +++ b/docs/rules/no-cycle.md @@ -0,0 +1,37 @@ +# import/no-cycle + +Ensures that there is no resolvable path back to this module via its dependencies. + +By default, this rule only detects cycles for ES6 imports, but see the [`no-unresolved` options](./no-unresolved.md#options) as this rule also supports the same `commonjs` and `amd` flags. However, these flags only impact which import types are _linted_; the +import/export infrastructure only registers `import` statements in dependencies, so +cycles created by `require` within imported modules may not be detected. + +This includes cycles of depth 1 (imported module imports me) to `Infinity`. + +```js +// dep-b.js +import './dep-a.js' + +export function b() { /* ... */ } + +// dep-a.js +import { b } from './dep-b.js' // reported: Dependency cycle detected. +``` + +This rule does _not_ detect imports that resolve directly to the linted module; +for that, see [`no-self-import`]. + + +## Rule Details + +## When Not To Use It + +This rule is computationally expensive. If you are pressed for lint time, or don't +think you have an issue with dependency cycles, you may not want this rule enabled. + +## Further Reading + +- [Original inspiring issue](https://github.com/benmosher/eslint-plugin-import/issues/941) +- Rule to detect that module imports itself: [`no-self-import`] + +[`no-self-import`]: ./no-self-import.md diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index c9148b7fa..b1bf7b3e9 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -4,20 +4,29 @@ */ import Exports from '../ExportMap' -import moduleVisitor, { optionsSchema } from 'eslint-module-utils/moduleVisitor' +import moduleVisitor, { makeOptionsSchema } from 'eslint-module-utils/moduleVisitor' import docsUrl from '../docsUrl' // todo: cache cycles / deep relationships for faster repeat evaluation module.exports = { meta: { docs: { url: docsUrl('no-cycle') }, - schema: [optionsSchema], + schema: [makeOptionsSchema({ + maxDepth:{ + description: 'maximum dependency depth to traverse', + type: 'integer', + minimum: 1, + }, + })], }, create: function (context) { const myPath = context.getFilename() if (myPath === '') return // can't cycle-check a non-file + const options = context.options[0] || {} + const maxDepth = options.maxDepth || Infinity + function checkSourceValue(sourceNode, importer) { const imported = Exports.get(sourceNode.value, context) @@ -29,19 +38,20 @@ module.exports = { return // no-self-import territory } - const untraversed = [{imported, route:[]}] + const untraversed = [{mget: () => imported, route:[]}] const traversed = new Set() - function detectCycle({imported: m, route}) { + function detectCycle({mget, route}) { + const m = mget() + if (m == null) return if (traversed.has(m.path)) return traversed.add(m.path) for (let [path, { getter, source }] of m.imports) { if (path === myPath) return true if (traversed.has(path)) continue - const deeper = getter() - if (deeper != null) { + if (route.length + 1 < maxDepth) { untraversed.push({ - imported: deeper, + mget: getter, route: route.concat(source), }) } diff --git a/utils/moduleVisitor.js b/utils/moduleVisitor.js index 2abcc8345..7bb980e45 100644 --- a/utils/moduleVisitor.js +++ b/utils/moduleVisitor.js @@ -90,9 +90,6 @@ exports.default = function visitModules(visitor, options) { /** * make an options schema for the module visitor, optionally * adding extra fields. - - * @param {[type]} additionalProperties [description] - * @return {[type]} [description] */ function makeOptionsSchema(additionalProperties) { const base = { From d81f48a2506182738409805f5272eff4d77c9348 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 26 Mar 2018 07:04:09 -0400 Subject: [PATCH 7/9] no-cycle: maxDepth tests + docs --- docs/rules/no-cycle.md | 40 ++++++++++++++++++++++++++++++------- tests/src/rules/no-cycle.js | 18 +++++++++++++++-- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/docs/rules/no-cycle.md b/docs/rules/no-cycle.md index 2c2a8e8ef..aa28fda0a 100644 --- a/docs/rules/no-cycle.md +++ b/docs/rules/no-cycle.md @@ -2,11 +2,8 @@ Ensures that there is no resolvable path back to this module via its dependencies. -By default, this rule only detects cycles for ES6 imports, but see the [`no-unresolved` options](./no-unresolved.md#options) as this rule also supports the same `commonjs` and `amd` flags. However, these flags only impact which import types are _linted_; the -import/export infrastructure only registers `import` statements in dependencies, so -cycles created by `require` within imported modules may not be detected. - -This includes cycles of depth 1 (imported module imports me) to `Infinity`. +This includes cycles of depth 1 (imported module imports me) to `Infinity`, if the +[`maxDepth`](#maxdepth) option is not set. ```js // dep-b.js @@ -24,10 +21,39 @@ for that, see [`no-self-import`]. ## Rule Details +### Options + +By default, this rule only detects cycles for ES6 imports, but see the [`no-unresolved` options](./no-unresolved.md#options) as this rule also supports the same `commonjs` and `amd` flags. However, these flags only impact which import types are _linted_; the +import/export infrastructure only registers `import` statements in dependencies, so +cycles created by `require` within imported modules may not be detected. + +#### `maxDepth` + +There is a `maxDepth` option available to prevent full expansion of very deep dependency trees: + +```js +/*eslint import/no-unresolved: [2, { maxDepth: 1 }]*/ + +// dep-c.js +import './dep-a.js' + +// dep-b.js +import './dep-c.js' + +export function b() { /* ... */ } + +// dep-a.js +import { b } from './dep-b.js' // not reported as the cycle is at depth 2 +``` + +This is not necessarily recommended, but available as a cost/benefit tradeoff mechanism +for reducing total project lint time, if needed. + ## When Not To Use It -This rule is computationally expensive. If you are pressed for lint time, or don't -think you have an issue with dependency cycles, you may not want this rule enabled. +This rule is comparatively computationally expensive. If you are pressed for lint +time, or don't think you have an issue with dependency cycles, you may not want +this rule enabled. ## Further Reading diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index 178e9438d..22b3682ac 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -11,7 +11,7 @@ const test = def => _test(Object.assign(def, { filename: testFilePath('./cycles/depth-zero.js'), })) -// describe.only("no-cycle", () => { +describe.only("no-cycle", () => { ruleTester.run('no-cycle', rule, { valid: [ // this rule doesn't care if the cycle length is 0 @@ -32,12 +32,21 @@ ruleTester.run('no-cycle', rule, { code: 'var bar = require("./bar")', filename: '', }), + test({ + code: 'import { foo } from "./depth-two"', + options: [{ maxDepth: 1 }], + }), ], invalid: [ test({ code: 'import { foo } from "./depth-one"', errors: [error(`Dependency cycle detected.`)], }), + test({ + code: 'import { foo } from "./depth-one"', + options: [{ maxDepth: 1 }], + errors: [error(`Dependency cycle detected.`)], + }), test({ code: 'const { foo } = require("./depth-one")', errors: [error(`Dependency cycle detected.`)], @@ -57,6 +66,11 @@ ruleTester.run('no-cycle', rule, { code: 'import { foo } from "./depth-two"', errors: [error(`Dependency cycle via ./depth-one:1`)], }), + test({ + code: 'import { foo } from "./depth-two"', + options: [{ maxDepth: 2 }], + errors: [error(`Dependency cycle via ./depth-one:1`)], + }), test({ code: 'const { foo } = require("./depth-two")', errors: [error(`Dependency cycle via ./depth-one:1`)], @@ -72,4 +86,4 @@ ruleTester.run('no-cycle', rule, { }), ], }) -// }) +}) From ad66aea712554a50a000ade565b81b0956ca1cfa Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Tue, 27 Mar 2018 20:06:32 -0400 Subject: [PATCH 8/9] smh. --- tests/files/cycles/depth-three-indirect.js | 2 +- tests/src/rules/no-cycle.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/files/cycles/depth-three-indirect.js b/tests/files/cycles/depth-three-indirect.js index 814562f44..f93ed00db 100644 --- a/tests/files/cycles/depth-three-indirect.js +++ b/tests/files/cycles/depth-three-indirect.js @@ -2,4 +2,4 @@ import './depth-two' export function bar() { return "side effects???" -} \ No newline at end of file +} diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index 22b3682ac..ae45ba36e 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -11,7 +11,7 @@ const test = def => _test(Object.assign(def, { filename: testFilePath('./cycles/depth-zero.js'), })) -describe.only("no-cycle", () => { +// describe.only("no-cycle", () => { ruleTester.run('no-cycle', rule, { valid: [ // this rule doesn't care if the cycle length is 0 @@ -86,4 +86,4 @@ ruleTester.run('no-cycle', rule, { }), ], }) -}) +// }) From ab44320f8f99972f7e07f31804616652d0d79c98 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Thu, 29 Mar 2018 20:21:58 -0400 Subject: [PATCH 9/9] changelog notes --- CHANGELOG.md | 3 +++ utils/CHANGELOG.md | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50b742d42..028d2a413 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ This project adheres to [Semantic Versioning](http://semver.org/). This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com). ## [Unreleased] +### Added +- Add [`no-cycle`] rule: reports import cycles. ## [2.9.0] - 2018-02-21 ### Added @@ -440,6 +442,7 @@ for info on changes for earlier releases. [`group-exports`]: ./docs/rules/group-exports.md [`no-self-import`]: ./docs/rules/no-self-import.md [`no-default-export`]: ./docs/rules/no-default-export.md +[`no-cycle`]: ./docs/rules/no-cycle.md [`memo-parser`]: ./memo-parser/README.md diff --git a/utils/CHANGELOG.md b/utils/CHANGELOG.md index cc9bc051b..5aaa3cc76 100644 --- a/utils/CHANGELOG.md +++ b/utils/CHANGELOG.md @@ -4,7 +4,11 @@ This project adheres to [Semantic Versioning](http://semver.org/). This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com). ## Unreleased - +### Changed +- `parse`: attach node locations by default. +- `moduleVisitor`: visitor now gets the full `import` statement node as a second + argument, so rules may report against the full statement / `require` call instead + of only the string literal node. ## v2.1.1 - 2017-06-22