fix(schematics): fix enforceModuleBoundaries check to properly detect incorrect relative imports

This commit is contained in:
vsavkin 2018-02-14 16:17:37 -05:00 committed by Victor Savkin
parent 222efe2f26
commit 70f7b608e7
4 changed files with 84 additions and 34 deletions

View File

@ -1,3 +1,11 @@
{ {
"singleQuote": true "singleQuote": true,
"overrides": [
{
"files": "*.ts",
"options": {
"parser": "typescript"
}
}
]
} }

View File

@ -15,7 +15,7 @@
"test:schematics": "yarn build && ./scripts/test_schematics.sh", "test:schematics": "yarn build && ./scripts/test_schematics.sh",
"test:nx": "yarn build && ./scripts/test_nx.sh", "test:nx": "yarn build && ./scripts/test_nx.sh",
"test": "yarn linknpm && ./scripts/test_nx.sh && ./scripts/test_schematics.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", "publish_npm": "./scripts/publish.sh",
"precommit": "yarn checkformat" "precommit": "yarn checkformat"
}, },

View File

@ -17,13 +17,30 @@ describe('Enforce Module Boundaries', () => {
expect(failures.length).toEqual(0); expect(failures.length).toEqual(0);
}); });
it('should error on a relative import of a library', () => { describe('relative imports', () => {
const failures = runRule({}, `import '../../../libs/mylib';`); 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.length).toEqual(1);
expect(failures[0].getFailure()).toEqual('library imports must start with @mycompany/'); 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', () => { it('should error on absolute imports into libraries without using the npm scope', () => {
const failures = runRule({}, `import 'libs/mylib';`); const failures = runRule({}, `import 'libs/mylib';`);
@ -95,7 +112,36 @@ function runRule(
ruleName: 'enforceModuleBoundaries' ruleName: 'enforceModuleBoundaries'
}; };
const sourceFile = ts.createSourceFile('proj/apps/myapp/src/main.ts', content, ts.ScriptTarget.Latest, true); const sourceFile = ts.createSourceFile(
const rule = new Rule(options, 'proj', 'mycompany', libNames, appNames); '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); return rule.apply(sourceFile);
} }

View File

@ -7,18 +7,20 @@ import { readFileSync } from 'fs';
export class Rule extends Lint.Rules.AbstractRule { export class Rule extends Lint.Rules.AbstractRule {
constructor( constructor(
options: IOptions, options: IOptions,
private path?: string, private readonly projectPath?: string,
private npmScope?: string, private readonly npmScope?: string,
private libNames?: string[], private readonly libNames?: string[],
private appNames?: string[] private readonly appNames?: string[],
private readonly roots?: string[]
) { ) {
super(options); super(options);
if (!path) { if (!projectPath) {
const cliConfig = this.readCliConfig(); const cliConfig = this.readCliConfig();
this.path = process.cwd(); this.projectPath = process.cwd();
this.npmScope = cliConfig.project.npmScope; this.npmScope = cliConfig.project.npmScope;
this.libNames = cliConfig.apps.filter(p => p.root.startsWith('libs/')).map(a => a.name); 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.appNames = cliConfig.apps.filter(p => p.root.startsWith('apps/')).map(a => a.name);
this.roots = cliConfig.apps.map(a => path.dirname(a.root));
} }
} }
@ -27,10 +29,11 @@ export class Rule extends Lint.Rules.AbstractRule {
new EnforceModuleBoundariesWalker( new EnforceModuleBoundariesWalker(
sourceFile, sourceFile,
this.getOptions(), this.getOptions(),
this.path, this.projectPath,
this.npmScope, this.npmScope,
this.libNames, this.libNames,
this.appNames this.appNames,
this.roots
) )
); );
} }
@ -47,9 +50,11 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker {
private projectPath: string, private projectPath: string,
private npmScope: string, private npmScope: string,
private libNames: string[], private libNames: string[],
private appNames: string[] private appNames: string[],
private roots: string[]
) { ) {
super(sourceFile, options); super(sourceFile, options);
this.roots = [...roots].sort((a, b) => a.length - b.length);
} }
public visitImportDeclaration(node: ts.ImportDeclaration) { public visitImportDeclaration(node: ts.ImportDeclaration) {
@ -103,29 +108,20 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker {
private isRelativeImportIntoAnotherProject(imp: string): boolean { private isRelativeImportIntoAnotherProject(imp: string): boolean {
if (!this.isRelative(imp)) return false; if (!this.isRelative(imp)) return false;
const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length); const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length);
const targetFile = path.resolve(path.dirname(sourceFile), imp); const targetFile = path.resolve(path.dirname(sourceFile), imp).substring(1); // remove leading slash
if (this.workspacePath(sourceFile) && this.workspacePath(targetFile)) { if (!this.libraryRoot()) return false;
if (this.parseProject(sourceFile) !== this.parseProject(targetFile)) { return !(targetFile.startsWith(`${this.libraryRoot()}/`) || targetFile === this.libraryRoot());
return true;
} }
}
return false; private libraryRoot(): string {
const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length + 1);
return this.roots.filter(r => sourceFile.startsWith(r))[0];
} }
private isAbsoluteImportIntoAnotherProject(imp: string): boolean { private isAbsoluteImportIntoAnotherProject(imp: string): boolean {
return imp.startsWith('libs/') || (imp.startsWith('/libs/') && imp.startsWith('apps/')) || imp.startsWith('/apps/'); return imp.startsWith('libs/') || (imp.startsWith('/libs/') && imp.startsWith('apps/')) || imp.startsWith('/apps/');
} }
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 isRelative(s: string): boolean { private isRelative(s: string): boolean {
return s.startsWith('.'); return s.startsWith('.');
} }