From 6c51c6284cba2d79336133dfe9cd659fbceb6cfa Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Wed, 2 Nov 2016 15:37:05 -0700 Subject: [PATCH] fix(aot): Use the proper path when statically analyzing lazy routes. Before, we were using paths relative to base at all time, but these might not be the paths we get in System.import(), therefore we have to keep the relative path. Also fix e2e tests. BREAKING CHANGES: Using relative paths might lead to path clashing. We now properly output an error in this case. Fixes #2452 Fixes #2900 --- package.json | 2 +- packages/angular-cli/package.json | 2 +- packages/angular-cli/upgrade/version.ts | 2 +- packages/webpack/src/loader.ts | 2 +- packages/webpack/src/plugin.ts | 124 ++++++++++++++---- tests/e2e/tests/build/styles/styles-array.ts | 7 +- .../generate/component/component-path-case.ts | 2 +- tests/e2e_runner.js | 6 +- 8 files changed, 109 insertions(+), 38 deletions(-) diff --git a/package.json b/package.json index a25c71729a40..0007f9407e5a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "angular-cli", - "version": "1.0.0-beta.19", + "version": "1.0.0-beta.19-3", "description": "CLI tool for Angular", "main": "packages/angular-cli/lib/cli/index.js", "trackingCode": "UA-8594346-19", diff --git a/packages/angular-cli/package.json b/packages/angular-cli/package.json index fb2a868312c7..6518b186838f 100644 --- a/packages/angular-cli/package.json +++ b/packages/angular-cli/package.json @@ -1,6 +1,6 @@ { "name": "angular-cli", - "version": "1.0.0-beta.19", + "version": "1.0.0-beta.19-3", "description": "CLI tool for Angular", "main": "lib/cli/index.js", "trackingCode": "UA-8594346-19", diff --git a/packages/angular-cli/upgrade/version.ts b/packages/angular-cli/upgrade/version.ts index 2560a397dfce..ac380c68500b 100644 --- a/packages/angular-cli/upgrade/version.ts +++ b/packages/angular-cli/upgrade/version.ts @@ -32,7 +32,7 @@ function _hasOldCliBuildFile() { export class Version { - constructor(private _version: string) {} + constructor(private _version: string = null) {} private _parse() { return this.isKnown() diff --git a/packages/webpack/src/loader.ts b/packages/webpack/src/loader.ts index 2a85fbfb7beb..51405c63c0ee 100644 --- a/packages/webpack/src/loader.ts +++ b/packages/webpack/src/loader.ts @@ -147,7 +147,7 @@ export function ngcLoader(source: string) { if (plugin && plugin instanceof AotPlugin) { const cb: any = this.async(); - plugin.done + Promise.resolve() .then(() => _removeDecorators(this.resource, source)) .then(sourceText => _replaceBootstrap(this.resource, sourceText, plugin)) .then(sourceText => { diff --git a/packages/webpack/src/plugin.ts b/packages/webpack/src/plugin.ts index ef4a600e6e9e..c545a0d3b7de 100644 --- a/packages/webpack/src/plugin.ts +++ b/packages/webpack/src/plugin.ts @@ -11,6 +11,7 @@ import {WebpackResourceLoader} from './resource_loader'; import {createResolveDependenciesFromContextMap} from './utils'; import {WebpackCompilerHost} from './compiler_host'; import {resolveEntryModuleFromMain} from './entry_resolver'; +import {StaticSymbol} from '@angular/compiler-cli'; /** @@ -25,8 +26,24 @@ export interface AotPluginOptions { } +export interface LazyRoute { + moduleRoute: ModuleRoute; + moduleRelativePath: string; + moduleAbsolutePath: string; +} + + +export interface LazyRouteMap { + [path: string]: LazyRoute; +} + + export class ModuleRoute { - constructor(public readonly path: string, public readonly className: string = null) {} + constructor(public readonly path: string, public readonly className: string = null) { + if (arguments.length == 1) { + [this.path, this.className] = path.split('#'); + } + } toString() { return `${this.path}#${this.className}`; @@ -57,6 +74,7 @@ export class AotPlugin { private _typeCheck: boolean = true; private _basePath: string; + private _genDir: string; constructor(options: AotPluginOptions) { @@ -68,7 +86,7 @@ export class AotPlugin { get compilerOptions() { return this._compilerOptions; } get done() { return this._donePromise; } get entryModule() { return this._entryModule; } - get genDir() { return this._basePath; } + get genDir() { return this._genDir; } get program() { return this._program; } get typeCheck() { return this._typeCheck; } @@ -113,6 +131,7 @@ export class AotPlugin { genDir }); this._basePath = basePath; + this._genDir = genDir; if (options.hasOwnProperty('typeChecking')) { this._typeCheck = options.typeChecking; @@ -168,9 +187,9 @@ export class AotPlugin { // Virtual file system. compiler.resolvers.normal.plugin('resolve', (request: any, cb?: () => void) => { - // Populate the file system cache with the virtual module. - this._compilerHost.populateWebpackResolver(compiler.resolvers.normal); - if (cb) { + if (request.request.match(/\.ts$/)) { + this.done.then(() => cb()); + } else { cb(); } }); @@ -227,49 +246,85 @@ export class AotPlugin { throw new Error(message); } }) + .then(() => { + // Populate the file system cache with the virtual module. + this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal); + }) .then(() => { // Process the lazy routes - this._lazyRoutes = - this._processNgModule(this._entryModule, null) - .map(module => ModuleRoute.fromString(module)) - .reduce((lazyRoutes: any, module: ModuleRoute) => { - lazyRoutes[`${module.path}.ngfactory`] = path.join( - this.genDir, module.path + '.ngfactory.ts'); - return lazyRoutes; - }, {}); + this._lazyRoutes = {}; + const allLazyRoutes = this._processNgModule(this._entryModule, null); + Object.keys(allLazyRoutes) + .forEach(k => { + const lazyRoute = allLazyRoutes[k]; + this._lazyRoutes[k + '.ngfactory'] = lazyRoute.moduleRelativePath + '.ngfactory.ts'; + }); }) - .then(() => cb(), (err: any) => cb(err)); + .then(() => cb(), (err: any) => { cb(err); }); } - private _resolveModule(module: ModuleRoute, containingFile: string) { + private _resolveModulePath(module: ModuleRoute, containingFile: string) { if (module.path.startsWith('.')) { return path.join(path.dirname(containingFile), module.path); } return module.path; } - private _processNgModule(module: ModuleRoute, containingFile: string | null): string[] { + private _processNgModule(module: ModuleRoute, containingFile: string | null): LazyRouteMap { const modulePath = containingFile ? module.path : ('./' + path.basename(module.path)); if (containingFile === null) { containingFile = module.path + '.ts'; } + const relativeModulePath = this._resolveModulePath(module, containingFile); - const resolvedModulePath = this._resolveModule(module, containingFile); const staticSymbol = this._reflectorHost .findDeclaration(modulePath, module.className, containingFile); const entryNgModuleMetadata = this.getNgModuleMetadata(staticSymbol); - const loadChildren = this.extractLoadChildren(entryNgModuleMetadata); - const result = loadChildren.map(route => { - return this._resolveModule(new ModuleRoute(route), resolvedModulePath); - }); + const loadChildrenRoute: LazyRoute[] = this.extractLoadChildren(entryNgModuleMetadata) + .map(route => { + const mr = ModuleRoute.fromString(route); + const relativePath = this._resolveModulePath(mr, relativeModulePath); + const absolutePath = path.join(this.genDir, relativePath); + return { + moduleRoute: mr, + moduleRelativePath: relativePath, + moduleAbsolutePath: absolutePath + }; + }); + const resultMap: LazyRouteMap = loadChildrenRoute + .reduce((acc: LazyRouteMap, curr: LazyRoute) => { + const key = curr.moduleRoute.path; + if (acc[key]) { + if (acc[key].moduleAbsolutePath != curr.moduleAbsolutePath) { + throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` + + 'loadChildren, but they point to different modules. Webpack cannot distinguish ' + + 'between the two based on context and would fail to load the proper one.'); + } + } else { + acc[key] = curr; + } + return acc; + }, {}); // Also concatenate every child of child modules. - for (const route of loadChildren) { - const childModule = ModuleRoute.fromString(route); - const children = this._processNgModule(childModule, resolvedModulePath + '.ts'); - result.push(...children); + for (const lazyRoute of loadChildrenRoute) { + const mr = lazyRoute.moduleRoute; + const children = this._processNgModule(mr, relativeModulePath); + Object.keys(children).forEach(p => { + const child = children[p]; + const key = child.moduleRoute.path; + if (resultMap[key]) { + if (resultMap[key].moduleAbsolutePath != child.moduleAbsolutePath) { + throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` + + 'loadChildren, but they point to different modules. Webpack cannot distinguish ' + + 'between the two based on context and would fail to load the proper one.'); + } + } else { + resultMap[key] = child; + } + }); } - return result; + return resultMap; } private getNgModuleMetadata(staticSymbol: ngCompiler.StaticSymbol) { @@ -281,10 +336,23 @@ export class AotPlugin { } private extractLoadChildren(ngModuleDecorator: any): any[] { - const routes = ngModuleDecorator.imports.reduce((mem: any[], m: any) => { + const routes = (ngModuleDecorator.imports || []).reduce((mem: any[], m: any) => { return mem.concat(this.collectRoutes(m.providers)); }, this.collectRoutes(ngModuleDecorator.providers)); - return this.collectLoadChildren(routes); + return this.collectLoadChildren(routes) + .concat((ngModuleDecorator.imports || []) + // Also recursively extractLoadChildren of modules we import. + .map((staticSymbol: any) => { + if (staticSymbol instanceof StaticSymbol) { + const entryNgModuleMetadata = this.getNgModuleMetadata(staticSymbol); + return this.extractLoadChildren(entryNgModuleMetadata); + } else { + return []; + } + }) + // Poor man's flat map. + .reduce((acc: any[], i: any) => acc.concat(i), [])) + .filter(x => !!x); } private collectRoutes(providers: any[]): any[] { diff --git a/tests/e2e/tests/build/styles/styles-array.ts b/tests/e2e/tests/build/styles/styles-array.ts index 8e5b6cf26007..184adc165f78 100644 --- a/tests/e2e/tests/build/styles/styles-array.ts +++ b/tests/e2e/tests/build/styles/styles-array.ts @@ -47,11 +47,10 @@ export default function() { .then(() => expectFileToMatch('dist/styles.bundle.js', /.upper.*.lower.*background.*#def/)) .then(() => ng('build', '--prod')) - .then(() => new Promise(() => - glob.sync('dist/styles.*.bundle.css').find(file => !!file))) + .then(() => glob.sync('dist/styles.*.bundle.css').find(file => !!file)) .then((styles) => - expectFileToMatch(styles, 'body { background-color: blue; }') - .then(() => expectFileToMatch(styles, 'p { background-color: red; }') + expectFileToMatch(styles, /body\s*\{\s*background-color:\s*blue\s*\}/) + .then(() => expectFileToMatch(styles, /p\s*\{\s*background-color:\s*red\s*\}/) .then(() => expectFileToMatch(styles, /.outer.*.inner.*background:\s*#[fF]+/)) .then(() => expectFileToMatch(styles, /.upper.*.lower.*background.*#def/))) ); diff --git a/tests/e2e/tests/generate/component/component-path-case.ts b/tests/e2e/tests/generate/component/component-path-case.ts index 942e2459a7c6..671e666bc86d 100644 --- a/tests/e2e/tests/generate/component/component-path-case.ts +++ b/tests/e2e/tests/generate/component/component-path-case.ts @@ -8,7 +8,7 @@ export default function() { const componentDir = join(rootDir.toLowerCase(), 'test-component'); createDir(rootDir); - return ng('generate', 'component', 'Upper-Dir', 'test-component') + return ng('generate', 'component', 'Upper-Dir/test-component') .then(() => expectFileToExist(componentDir)) .then(() => expectFileToExist(join(componentDir, 'test-component.component.ts'))) .then(() => expectFileToExist(join(componentDir, 'test-component.component.spec.ts'))) diff --git a/tests/e2e_runner.js b/tests/e2e_runner.js index cf3523b67884..bc9b08152437 100644 --- a/tests/e2e_runner.js +++ b/tests/e2e_runner.js @@ -74,6 +74,7 @@ if (testsToRun.length == allTests.length) { } testsToRun.reduce((previous, relativeName) => { + console.log(relativeName); // Make sure this is a windows compatible path. let absoluteName = path.join(e2eRoot, relativeName); if (/^win/.test(process.platform)) { @@ -95,6 +96,7 @@ testsToRun.reduce((previous, relativeName) => { return Promise.resolve() .then(() => printHeader(currentFileName)) .then(() => fn(argv, () => clean = false)) + .then(() => console.log(' ----')) .then(() => { // Only clean after a real test, not a setup step. Also skip cleaning if the test // requested an exception. @@ -104,7 +106,9 @@ testsToRun.reduce((previous, relativeName) => { }) .then(() => printFooter(currentFileName, start), (err) => { - printFooter(currentFileName, start); throw err; + printFooter(currentFileName, start); + console.error(err); + throw err; }); }); }, Promise.resolve())