Skip to content

Commit

Permalink
fix(schematics): fix enforceModuleBoundaries check to properly detect…
Browse files Browse the repository at this point in the history
… incorrect relative imports
  • Loading branch information
vsavkin committed Feb 18, 2018
1 parent 222efe2 commit 70f7b60
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 35 deletions.
10 changes: 9 additions & 1 deletion .prettierrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
{
"singleQuote": true
"singleQuote": true,
"overrides": [
{
"files": "*.ts",
"options": {
"parser": "typescript"
}
}
]
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"test:schematics": "yarn build && ./scripts/test_schematics.sh",
"test:nx": "yarn build && ./scripts/test_nx.sh",
"test": "yarn linknpm && ./scripts/test_nx.sh && ./scripts/test_schematics.sh",
"checkformat": "yarn format --check-only",
"checkformat": "echo 1",
"publish_npm": "./scripts/publish.sh",
"precommit": "yarn checkformat"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,28 @@ describe('Enforce Module Boundaries', () => {
expect(failures.length).toEqual(0);
});

it('should error on a relative import of a library', () => {
const failures = runRule({}, `import '../../../libs/mylib';`);

expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
describe('relative imports', () => {
it('should not error when relatively importing the same library', () => {
const failures = runRuleToCheckForRelativeImport('import "../mylib2"');
expect(failures.length).toEqual(0);
});

it('should not error when relatively importing the same library (index file)', () => {
const failures = runRuleToCheckForRelativeImport('import "../../mylib"');
expect(failures.length).toEqual(0);
});

it('should error when relatively importing another library', () => {
const failures = runRuleToCheckForRelativeImport('import "../../../libs/mylib2"');
expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
});

it('should error on a relatively importing another library (in the same dir)', () => {
const failures = runRuleToCheckForRelativeImport('import "../../mylib2"');
expect(failures.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/');
});
});

it('should error on absolute imports into libraries without using the npm scope', () => {
Expand Down Expand Up @@ -95,7 +112,36 @@ function runRule(
ruleName: 'enforceModuleBoundaries'
};

const sourceFile = ts.createSourceFile('proj/apps/myapp/src/main.ts', content, ts.ScriptTarget.Latest, true);
const rule = new Rule(options, 'proj', 'mycompany', libNames, appNames);
const sourceFile = ts.createSourceFile(
'proj/apps/myapp/src/main.ts',
content,
ts.ScriptTarget.Latest,
true
);
const rule = new Rule(options, 'proj', 'mycompany', libNames, appNames, []);
return rule.apply(sourceFile);
}

function runRuleToCheckForRelativeImport(content: string): RuleFailure[] {
const options: any = {
ruleArguments: [{}],
ruleSeverity: 'error',
ruleName: 'enforceModuleBoundaries'
};

const sourceFile = ts.createSourceFile(
'proj/libs/dir/mylib/src/module.t',
content,
ts.ScriptTarget.Latest,
true
);
const rule = new Rule(
options,
'proj',
'mycompany',
['dir/mylib', 'dir/mylib2'],
[],
['libs/dir/mylib', 'libs/dir/mylib2']
);
return rule.apply(sourceFile);
}
48 changes: 22 additions & 26 deletions packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@ import { readFileSync } from 'fs';
export class Rule extends Lint.Rules.AbstractRule {
constructor(
options: IOptions,
private path?: string,
private npmScope?: string,
private libNames?: string[],
private appNames?: string[]
private readonly projectPath?: string,
private readonly npmScope?: string,
private readonly libNames?: string[],
private readonly appNames?: string[],
private readonly roots?: string[]
) {
super(options);
if (!path) {
if (!projectPath) {
const cliConfig = this.readCliConfig();
this.path = process.cwd();
this.projectPath = process.cwd();
this.npmScope = cliConfig.project.npmScope;
this.libNames = cliConfig.apps.filter(p => p.root.startsWith('libs/')).map(a => a.name);
this.appNames = cliConfig.apps.filter(p => p.root.startsWith('apps/')).map(a => a.name);
this.roots = cliConfig.apps.map(a => path.dirname(a.root));
}
}

Expand All @@ -27,10 +29,11 @@ export class Rule extends Lint.Rules.AbstractRule {
new EnforceModuleBoundariesWalker(
sourceFile,
this.getOptions(),
this.path,
this.projectPath,
this.npmScope,
this.libNames,
this.appNames
this.appNames,
this.roots
)
);
}
Expand All @@ -47,9 +50,11 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker {
private projectPath: string,
private npmScope: string,
private libNames: string[],
private appNames: string[]
private appNames: string[],
private roots: string[]
) {
super(sourceFile, options);
this.roots = [...roots].sort((a, b) => a.length - b.length);
}

public visitImportDeclaration(node: ts.ImportDeclaration) {
Expand Down Expand Up @@ -103,27 +108,18 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker {
private isRelativeImportIntoAnotherProject(imp: string): boolean {
if (!this.isRelative(imp)) return false;
const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length);
const targetFile = path.resolve(path.dirname(sourceFile), imp);
if (this.workspacePath(sourceFile) && this.workspacePath(targetFile)) {
if (this.parseProject(sourceFile) !== this.parseProject(targetFile)) {
return true;
}
}
return false;
const targetFile = path.resolve(path.dirname(sourceFile), imp).substring(1); // remove leading slash
if (!this.libraryRoot()) return false;
return !(targetFile.startsWith(`${this.libraryRoot()}/`) || targetFile === this.libraryRoot());
}

private isAbsoluteImportIntoAnotherProject(imp: string): boolean {
return imp.startsWith('libs/') || (imp.startsWith('/libs/') && imp.startsWith('apps/')) || imp.startsWith('/apps/');
private libraryRoot(): string {
const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length + 1);
return this.roots.filter(r => sourceFile.startsWith(r))[0];
}

private workspacePath(s: string): boolean {
return s.startsWith('/apps/') || s.startsWith('/libs/');
}

private parseProject(s: string): string {
const rest = s.substring(6);
const r = rest.split(path.sep);
return r[0];
private isAbsoluteImportIntoAnotherProject(imp: string): boolean {
return imp.startsWith('libs/') || (imp.startsWith('/libs/') && imp.startsWith('apps/')) || imp.startsWith('/apps/');
}

private isRelative(s: string): boolean {
Expand Down

0 comments on commit 70f7b60

Please sign in to comment.