From 56788ba3d1a8220455b03f0c766963ae8179ce81 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Mon, 12 Mar 2018 19:54:04 -0400 Subject: [PATCH] feat(schematics): show warnings about importing lazy loadable libraries without forcing users configure them in tslint.json --- e2e/schematics/command-line.test.ts | 7 +- .../files/__directory__/tslint.json | 1 - .../src/collection/workspace/index.ts | 2 +- .../schematics/src/command-line/shared.ts | 2 +- .../nxEnforceModuleBoundariesRule.spec.ts | 266 +++++++++++------- .../tslint/nxEnforceModuleBoundariesRule.ts | 130 +++++---- 6 files changed, 249 insertions(+), 159 deletions(-) diff --git a/e2e/schematics/command-line.test.ts b/e2e/schematics/command-line.test.ts index 3163fcea075d4..2db69023bec93 100644 --- a/e2e/schematics/command-line.test.ts +++ b/e2e/schematics/command-line.test.ts @@ -6,11 +6,11 @@ describe('Command line', () => { () => { newProject(); newApp('myapp'); + newApp('myapp2'); newLib('mylib'); newLib('lazylib'); const tslint = JSON.parse(readFile('tslint.json')); - tslint.rules['nx-enforce-module-boundaries'][1].lazyLoad.push('lazylib'); updateFile('tslint.json', JSON.stringify(tslint, null, 2)); updateFile( @@ -19,8 +19,9 @@ describe('Command line', () => { import '../../../libs/mylib'; import '@proj/lazylib'; import '@proj/mylib/deep'; - import '@proj/myapp'; - import '@proj/myapp/main'; + import '@proj/myapp2'; + + const s = {loadChildren: '@proj/lazylib'}; ` ); diff --git a/packages/schematics/src/collection/application/files/__directory__/tslint.json b/packages/schematics/src/collection/application/files/__directory__/tslint.json index 8438036b72772..d57f2f3c419ba 100644 --- a/packages/schematics/src/collection/application/files/__directory__/tslint.json +++ b/packages/schematics/src/collection/application/files/__directory__/tslint.json @@ -94,7 +94,6 @@ "nx-enforce-module-boundaries": [ true, { - "lazyLoad": [], "allow": [] } ] diff --git a/packages/schematics/src/collection/workspace/index.ts b/packages/schematics/src/collection/workspace/index.ts index 55dfc7b49448f..28e442f54b8b6 100644 --- a/packages/schematics/src/collection/workspace/index.ts +++ b/packages/schematics/src/collection/workspace/index.ts @@ -187,7 +187,7 @@ function updateTsLintJson(options: Schema) { json[key] = undefined; }); json.rulesDirectory.push('node_modules/@nrwl/schematics/src/tslint'); - json['nx-enforce-module-boundaries'] = [true, { lazyLoad: [], allow: [] }]; + json['nx-enforce-module-boundaries'] = [true, { allow: [] }]; }); return host; }; diff --git a/packages/schematics/src/command-line/shared.ts b/packages/schematics/src/command-line/shared.ts index 48d50ab2862ce..5b0a6ceb1ae06 100644 --- a/packages/schematics/src/command-line/shared.ts +++ b/packages/schematics/src/command-line/shared.ts @@ -41,7 +41,7 @@ function getFilesFromShash(sha1: string, sha2: string): string[] { .filter(a => a.length > 0); } -function getProjectNodes(config) { +export function getProjectNodes(config) { return (config.apps ? config.apps : []).filter(p => p.name !== '$workspaceRoot').map(p => { return { name: p.name, diff --git a/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.spec.ts b/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.spec.ts index 04672b2b31d65..4edb74a3416d6 100644 --- a/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.spec.ts +++ b/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.spec.ts @@ -2,122 +2,217 @@ import { RuleFailure } from 'tslint'; import * as ts from 'typescript'; import { Rule } from './nxEnforceModuleBoundariesRule'; +import {Dependency, DependencyType, ProjectNode, ProjectType} from '../command-line/affected-apps'; describe('Enforce Module Boundaries', () => { it('should not error when everything is in order', () => { const failures = runRule( { allow: ['@mycompany/mylib/deep'] }, + `${process.cwd()}/proj/apps/myapp/src/main.ts`, ` - import '@mycompany/mylib'; - import '@mycompany/mylib/deep'; - import '../blah'; - ` + import '@mycompany/mylib'; + import '@mycompany/mylib/deep'; + import '../blah'; + `, + [ + { + name: 'myapp', + root: 'libs/myapp/src', + type: ProjectType.app, + files: [ + `apps/myapp/src/main.ts`, + `apps/myapp/blah.ts` + ] + }, + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [ + `libs/mylib/index.ts`, + `libs/mylib/deep.ts` + ] + } + ] ); expect(failures.length).toEqual(0); }); - it('should not error when lib name prefix collides with name of lazy loaded lib', () => { - const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/mylib-not-lazy';`); - expect(failures.length).toEqual(0); - }); - describe('relative imports', () => { it('should not error when relatively importing the same library', () => { - const failures = runRuleToCheckForRelativeImport('import "../mylib2"'); + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "../other"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [ + `libs/mylib/src/main.ts`, + `libs/mylib/other.ts` + ] + } + ] + ); 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 not error when relatively importing the same library (lib name prefix collision)', () => { - const failures = runRuleToCheckForRelativeImport( - 'import "../some-comp"', - '/proj/libs/dir/mylib2/src/module.t' + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "../other"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [ + `libs/mylib/src/main.ts`, + `libs/mylib/other/index.ts` + ] + } + ] ); 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/'); + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "../other"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [`libs/mylib/src/main.ts`] + }, + { + name: 'other', + root: 'libs/other/src', + type: ProjectType.lib, + files: [] + } + ] + ); + expect(failures[0].getFailure()).toEqual( + 'library imports must start with @mycompany/' + ); }); }); it('should error on absolute imports into libraries without using the npm scope', () => { - const failures = runRule({}, `import 'libs/mylib';`); + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "libs/src/other.ts"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [ + `libs/mylib/src/main.ts`, + `libs/mylib/src/other.ts` + ] + } + ] + ); 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 about deep imports into libraries', () => { - const failures = runRule({}, `import '@mycompany/mylib/blah';`); - - expect(failures.length).toEqual(1); - expect(failures[0].getFailure()).toEqual('deep imports into libraries are forbidden'); - }); - - it('should not error about deep imports when libs contain the same prefix', () => { - let failures = runRule( + const failures = runRule( {}, - `import '@mycompany/reporting-dashboard-ui'; - import '@mycompany/reporting-other'; - import '@mycompany/reporting'; - `, - ['reporting', 'reporting-dashboard-ui'] + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "@mycompany/other/blah"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [`libs/mylib/src/main.ts`] + }, + { + name: 'other', + root: 'libs/other/src', + type: ProjectType.lib, + files: [`libs/other/blah.ts`] + } + ] ); - - expect(failures.length).toEqual(0); - - // Make sure it works regardless of order of app names list - failures = runRule( - {}, - `import '@mycompany/reporting-dashboard-ui'; - import '@mycompany/reporting-other'; - import '@mycompany/reporting';`, - ['reporting-dashboard-ui', 'reporting'] + expect(failures[0].getFailure()).toEqual( + 'deep imports into libraries are forbidden' ); - - expect(failures.length).toEqual(0); }); it('should error on importing a lazy-loaded library', () => { - const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/mylib';`); - - expect(failures.length).toEqual(1); + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "@mycompany/other";', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [`libs/mylib/src/main.ts`] + }, + { + name: 'other', + root: 'libs/other/src', + type: ProjectType.lib, + files: [`libs/other/index.ts`] + } + ], + { + mylib: [{projectName: 'other', type: DependencyType.loadChildren}] + } + ); expect(failures[0].getFailure()).toEqual('imports of lazy-loaded libraries are forbidden'); }); it('should error on importing an app', () => { - const failures = runRule({ lazyLoad: ['mylib'] }, `import '@mycompany/myapp';`, [], ['myapp']); - - expect(failures.length).toEqual(1); - expect(failures[0].getFailure()).toEqual('imports of apps are forbidden'); - }); - - it('should error on importing a lib that has the same prefix as an app', () => { - const noFailures = runRule({ lazyLoad: [] }, `import '@mycompany/myapp/mylib';`, ['myapp/mylib'], ['myapp']); - - expect(noFailures.length).toEqual(0); + const failures = runRule( + {}, + `${process.cwd()}/proj/libs/mylib/src/main.ts`, + 'import "@mycompany/myapp"', + [ + { + name: 'mylib', + root: 'libs/mylib/src', + type: ProjectType.lib, + files: [`libs/mylib/src/main.ts`] + }, + { + name: 'myapp', + root: 'apps/myapp/src', + type: ProjectType.app, + files: [`apps/myapp/index.ts`] + } + ] + ); + expect(failures[0].getFailure()).toEqual( + 'imports of apps are forbidden' + ); }); }); function runRule( ruleArguments: any, + contentPath: string, content: string, - libNames: string[] = ['mylib'], - appNames: string[] = [] + projectNodes: ProjectNode[], + deps: { [projectName: string]: Dependency[] } = {} ): RuleFailure[] { const options: any = { ruleArguments: [ruleArguments], @@ -126,38 +221,11 @@ function runRule( }; const sourceFile = ts.createSourceFile( - 'proj/apps/myapp/src/main.ts', + contentPath, content, ts.ScriptTarget.Latest, true ); - const rule = new Rule(options, 'proj', 'mycompany', libNames, appNames, []); - return rule.apply(sourceFile); -} - -function runRuleToCheckForRelativeImport( - content: string, - sourceFilePath = '/proj/libs/dir/mylib/src/module.t' -): RuleFailure[] { - const options: any = { - ruleArguments: [{}], - ruleSeverity: 'error', - ruleName: 'enforceModuleBoundaries' - }; - - const sourceFile = ts.createSourceFile( - sourceFilePath, - content, - ts.ScriptTarget.Latest, - true - ); - const rule = new Rule( - options, - '/proj', - 'mycompany', - ['dir/mylib', 'dir/mylib2'], - [], - ['libs/dir/mylib', 'libs/dir/mylib2'] - ); + const rule = new Rule(options, `${process.cwd()}/proj`, 'mycompany', projectNodes, deps); return rule.apply(sourceFile); -} +} \ No newline at end of file diff --git a/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.ts b/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.ts index d14417598cd01..0e2633c8e4860 100644 --- a/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.ts +++ b/packages/schematics/src/tslint/nxEnforceModuleBoundariesRule.ts @@ -4,24 +4,24 @@ import { IOptions } from 'tslint'; import * as ts from 'typescript'; import { readFileSync } from 'fs'; import * as appRoot from 'app-root-path'; +import {getProjectNodes} from '../command-line/shared'; +import {dependencies, Dependency, DependencyType, ProjectNode, ProjectType} from '../command-line/affected-apps'; export class Rule extends Lint.Rules.AbstractRule { constructor( options: IOptions, private readonly projectPath?: string, private readonly npmScope?: string, - private readonly libNames?: string[], - private readonly appNames?: string[], - private readonly roots?: string[] + private readonly projectNodes?: ProjectNode[], + private readonly deps?: { [projectName: string]: Dependency[] }, ) { super(options); if (!projectPath) { this.projectPath = appRoot.path; const cliConfig = this.readCliConfig(this.projectPath); 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)); + this.projectNodes = getProjectNodes(cliConfig); + this.deps = dependencies(this.npmScope, this.projectNodes, f => readFileSync(f).toString()); } } @@ -32,9 +32,8 @@ export class Rule extends Lint.Rules.AbstractRule { this.getOptions(), this.projectPath, this.npmScope, - this.libNames, - this.appNames, - this.roots + this.projectNodes, + this.deps ) ); } @@ -48,11 +47,10 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { constructor( sourceFile: ts.SourceFile, options: IOptions, - private projectPath: string, - private npmScope: string, - private libNames: string[], - private appNames: string[], - private roots: string[] + private readonly projectPath: string, + private readonly npmScope: string, + private readonly projectNodes: ProjectNode[], + private readonly deps: { [projectName: string]: Dependency[] } ) { super(sourceFile, options); } @@ -62,9 +60,6 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { const allow: string[] = Array.isArray(this.getOptions()[0].allow) ? this.getOptions()[0].allow.map(a => `${a}`) : []; - const lazyLoad: string[] = Array.isArray(this.getOptions()[0].lazyLoad) - ? this.getOptions()[0].lazyLoad.map(a => `${a}`) - : []; // whitelisted import => return if (allow.indexOf(imp) > -1) { @@ -72,61 +67,84 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { return; } - const lazyLoaded = lazyLoad.filter( - l => imp.startsWith(`@${this.npmScope}/${l}/`) || imp === `@${this.npmScope}/${l}` - )[0]; - if (lazyLoaded) { - this.addFailureAt(node.getStart(), node.getWidth(), 'imports of lazy-loaded libraries are forbidden'); - return; - } - - if (this.libNames.filter(l => imp === `@${this.npmScope}/${l}`).length > 0) { - super.visitImportDeclaration(node); - return; - } - if (this.isRelativeImportIntoAnotherProject(imp) || this.isAbsoluteImportIntoAnotherProject(imp)) { this.addFailureAt(node.getStart(), node.getWidth(), `library imports must start with @${this.npmScope}/`); return; } - const deepImport = this.libNames.filter(l => imp.startsWith(`@${this.npmScope}/${l}/`))[0]; - if (deepImport) { - this.addFailureAt(node.getStart(), node.getWidth(), 'deep imports into libraries are forbidden'); - return; - } - - const appImport = this.appNames.filter( - a => imp.startsWith(`@${this.npmScope}/${a}/`) || imp === `@${this.npmScope}/${a}` - )[0]; - if (appImport) { - this.addFailureAt(node.getStart(), node.getWidth(), 'imports of apps are forbidden'); - return; + if (imp.startsWith(`@${this.npmScope}/`)) { + const name = imp.split('/')[1]; + const sourceProject = this.findSourceProject(); + const targetProject = this.findProjectUsingName(name); + + if (sourceProject === targetProject || !targetProject) { + super.visitImportDeclaration(node); + return; + } + + if (targetProject.type === ProjectType.app) { + this.addFailureAt(node.getStart(), node.getWidth(), 'imports of apps are forbidden'); + return; + } + + if (imp.split('/').length > 2) { + this.addFailureAt(node.getStart(), node.getWidth(), 'deep imports into libraries are forbidden'); + return; + } + + if (this.onlyLoadChildren(sourceProject.name, targetProject.name, [])) { + this.addFailureAt(node.getStart(), node.getWidth(), 'imports of lazy-loaded libraries are forbidden'); + return; + } } super.visitImportDeclaration(node); } + private onlyLoadChildren(source: string, target: string, path: string[]) { + if (path.indexOf(source) > -1) return false; + return (this.deps[source] || []).filter(d => { + if (d.type !== DependencyType.loadChildren) return false; + if (d.projectName === target) return true; + return this.onlyLoadChildren(d.projectName, target, [...path, source]); + }).length > 0; + } + private isRelativeImportIntoAnotherProject(imp: string): boolean { if (!this.isRelative(imp)) return false; - const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length); - /** - * include projectPath for resolve (windows compatibility, eg. c:\) - * and remove including leading slash afterwards for import comparison. - * be sure separator is '/' like at the import statements. - **/ + const targetFile = path - .resolve(this.projectPath + path.dirname(sourceFile), imp) + .resolve(path.join(this.projectPath, path.dirname(this.getSourceFilePath())), imp) .split(path.sep) .join('/') .substring(this.projectPath.length + 1); - if (!this.libraryRoot()) return false; - return !(targetFile.startsWith(`${this.libraryRoot()}/`) || targetFile === this.libraryRoot()); + const sourceProject = this.findSourceProject(); + + let targetProject = this.findProjectUsingFile(targetFile); + if (!targetProject) { + targetProject = this.findProjectUsingFile(path.join(targetFile, 'index')); + } + return sourceProject !== targetProject || targetProject === undefined; + } + + private findSourceProject() { + return this.projectNodes.filter(n => { + return n.files.filter(f => removeExt(f) === removeExt(this.getSourceFilePath()))[0]; + })[0]; + } + + private getSourceFilePath() { + return this.getSourceFile().fileName.substring(this.projectPath.length + 1); } - private libraryRoot(): string { - const sourceFile = this.getSourceFile().fileName.substring(this.projectPath.length + 1); - return this.roots.filter(r => sourceFile.startsWith(`${r}/`))[0]; + private findProjectUsingFile(file: string) { + return this.projectNodes.filter(n => { + return n.files.filter(f => removeExt(f) === file)[0]; + })[0]; + } + + private findProjectUsingName(name: string) { + return this.projectNodes.filter(n => n.name === name)[0]; } private isAbsoluteImportIntoAnotherProject(imp: string): boolean { @@ -137,3 +155,7 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { return s.startsWith('.'); } } + +function removeExt(file:string): string { + return file.replace(/\.[^/.]+$/, ""); +} \ No newline at end of file