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

fix(schematics): fix enforceModuleBoundaries check to properly detect… #271

Merged
merged 1 commit into from
Feb 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
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