Skip to content
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

Added search-depth-limit parameter #23

Merged
merged 2 commits into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 46 additions & 20 deletions noCircularImportsRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,16 @@ interface Options {

/** @internal */
rootDir: string

/**
* Maximum search depth to check for in generating a list of all cycles.
*/
searchDepthLimit: number
}

const OPTION_SEARCH_DEPTH_LIMIT = "search-depth-limit"
const OPTION_SEARCH_DEPTH_LIMIT_DEFAULT = 50

export class Rule extends Lint.Rules.TypedRule {
static FAILURE_STRING = 'circular import detected'

Expand All @@ -18,9 +26,23 @@ export class Rule extends Lint.Rules.TypedRule {
description: 'Disallows circular imports.',
rationale: Lint.Utils.dedent`
Circular dependencies cause hard-to-catch runtime exceptions.`,
optionsDescription: 'Not configurable.',
options: null,
optionExamples: ['true'],
optionsDescription: Lint.Utils.dedent`
A single argument, ${OPTION_SEARCH_DEPTH_LIMIT}, may be provided, and defaults to ${OPTION_SEARCH_DEPTH_LIMIT_DEFAULT}.
It limits the depth of cycle reporting to a fixed size limit for a list of files.
This helps improve performance, as most cycles do not surpass a few related files.
`,
options: {
properties: {
[OPTION_SEARCH_DEPTH_LIMIT]: {
type: "number"
}
},
type: "object"
},
optionExamples: [
['true'],
['true', { [OPTION_SEARCH_DEPTH_LIMIT]: 50 }]
],
type: 'functionality',
typescriptOnly: false
}
Expand All @@ -37,6 +59,7 @@ export class Rule extends Lint.Rules.TypedRule {
{
compilerOptions,
rootDir: compilerOptions.rootDir || process.cwd(),
searchDepthLimit: this.ruleArguments[0]["search-depth-limit"] || OPTION_SEARCH_DEPTH_LIMIT
},
program.getTypeChecker())
}
Expand Down Expand Up @@ -113,6 +136,26 @@ function walk(context: Lint.WalkContext<Options>) {

addToGraph(fileName, resolvedImportFileName, node)
}

function getAllCycles(moduleName: string, accumulator: string[] = [], iterationDepth = 0): string[][] {
const moduleImport = imports.get(moduleName)
if (!moduleImport) return []
if (accumulator.indexOf(moduleName) !== -1)
return [accumulator]

if (iterationDepth >= context.options.searchDepthLimit)
return []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a console.warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally no. Large codebases will hit this a lot.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, when I have a cycle in a big codebase, will I ever find out about it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you think the log wouldn't be useful, since there likely isn't a cycle (just deep dep graph?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I'm thinking. From working in code bases with a few hundred or a few thousand files, most of them just import a few around their own directory. Even 50 is pretty extreme.

...then again... you're right that if this results in nothing, but we know there was at least one cycle and it happens to be ridiculously long, I don't know what we could do intelligently show that.

Sorry for the brevity, on mobile :)


const all: string[][] = []
for (const imp of Array.from(moduleImport.keys())) {
const c = getAllCycles(imp, accumulator.concat(moduleName), iterationDepth + 1)

if (c.length)
all.push(...c)
}

return all
}
}

function addToGraph(thisFileName: string, importCanonicalName: string, node: ts.Node) {
Expand Down Expand Up @@ -146,20 +189,3 @@ function checkCycle(moduleName: string): boolean {

return false
}

function getAllCycles(moduleName: string, accumulator: string[] = []): string[][] {
const moduleImport = imports.get(moduleName)
if (!moduleImport) return []
if (accumulator.indexOf(moduleName) !== -1)
return [accumulator]

const all: string[][] = []
for (const imp of Array.from(moduleImport.keys())) {
const c = getAllCycles(imp, accumulator.concat(moduleName))

if (c.length)
all.push(...c)
}

return all
}
1 change: 1 addition & 0 deletions test/case-long-stack/four.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import * as one from "./one";
1 change: 1 addition & 0 deletions test/case-long-stack/one.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import * as two from "./two";
1 change: 1 addition & 0 deletions test/case-long-stack/three.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import * as three from "./three";
1 change: 1 addition & 0 deletions test/case-long-stack/two.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import * as two from "./two";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you trying to test with these files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These validate that the search-depth-limit option is respected.

one imports from two imports from three imports from four imports from one for a depth of four. Adding the option to tslint.json means these shouldn't cause a lint complaint, which is why I didn't modify the list of expected complaints.

4 changes: 3 additions & 1 deletion test/tslint.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"rulesDirectory": ["../"],
"rules": {
"no-circular-imports": true
"no-circular-imports": [true, {
"search-depth-limit": 3
}]
}
}