Skip to content

Commit 88131a0

Browse files
authored
fix(aot): Use the proper path when statically analyzing lazy routes. (#2992)
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 #2735 Fixes #2900
1 parent 37a1225 commit 88131a0

File tree

6 files changed

+99
-32
lines changed

6 files changed

+99
-32
lines changed

package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "angular-cli",
3-
"version": "1.0.0-beta.19",
3+
"version": "1.0.0-beta.19-3",
44
"description": "CLI tool for Angular",
55
"main": "packages/angular-cli/lib/cli/index.js",
66
"trackingCode": "UA-8594346-19",

packages/angular-cli/package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "angular-cli",
3-
"version": "1.0.0-beta.19",
3+
"version": "1.0.0-beta.19-3",
44
"description": "CLI tool for Angular",
55
"main": "lib/cli/index.js",
66
"trackingCode": "UA-8594346-19",

packages/angular-cli/upgrade/version.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ function _hasOldCliBuildFile() {
3232

3333

3434
export class Version {
35-
constructor(private _version: string) {}
35+
constructor(private _version: string = null) {}
3636

3737
private _parse() {
3838
return this.isKnown()

packages/webpack/src/loader.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ export function ngcLoader(source: string) {
147147
if (plugin && plugin instanceof AotPlugin) {
148148
const cb: any = this.async();
149149

150-
plugin.done
150+
Promise.resolve()
151151
.then(() => _removeDecorators(this.resource, source))
152152
.then(sourceText => _replaceBootstrap(this.resource, sourceText, plugin))
153153
.then(sourceText => {

packages/webpack/src/plugin.ts

+91-27
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {WebpackResourceLoader} from './resource_loader';
1111
import {createResolveDependenciesFromContextMap} from './utils';
1212
import {WebpackCompilerHost} from './compiler_host';
1313
import {resolveEntryModuleFromMain} from './entry_resolver';
14+
import {StaticSymbol} from '@angular/compiler-cli';
1415

1516

1617
/**
@@ -25,6 +26,18 @@ export interface AotPluginOptions {
2526
}
2627

2728

29+
export interface LazyRoute {
30+
moduleRoute: ModuleRoute;
31+
moduleRelativePath: string;
32+
moduleAbsolutePath: string;
33+
}
34+
35+
36+
export interface LazyRouteMap {
37+
[path: string]: LazyRoute;
38+
}
39+
40+
2841
export class ModuleRoute {
2942
constructor(public readonly path: string, public readonly className: string = null) {}
3043

@@ -57,6 +70,7 @@ export class AotPlugin {
5770

5871
private _typeCheck: boolean = true;
5972
private _basePath: string;
73+
private _genDir: string;
6074

6175

6276
constructor(options: AotPluginOptions) {
@@ -68,7 +82,7 @@ export class AotPlugin {
6882
get compilerOptions() { return this._compilerOptions; }
6983
get done() { return this._donePromise; }
7084
get entryModule() { return this._entryModule; }
71-
get genDir() { return this._basePath; }
85+
get genDir() { return this._genDir; }
7286
get program() { return this._program; }
7387
get typeCheck() { return this._typeCheck; }
7488

@@ -113,6 +127,7 @@ export class AotPlugin {
113127
genDir
114128
});
115129
this._basePath = basePath;
130+
this._genDir = genDir;
116131

117132
if (options.hasOwnProperty('typeChecking')) {
118133
this._typeCheck = options.typeChecking;
@@ -168,9 +183,9 @@ export class AotPlugin {
168183

169184
// Virtual file system.
170185
compiler.resolvers.normal.plugin('resolve', (request: any, cb?: () => void) => {
171-
// Populate the file system cache with the virtual module.
172-
this._compilerHost.populateWebpackResolver(compiler.resolvers.normal);
173-
if (cb) {
186+
if (request.request.match(/\.ts$/)) {
187+
this.done.then(() => cb());
188+
} else {
174189
cb();
175190
}
176191
});
@@ -227,49 +242,85 @@ export class AotPlugin {
227242
throw new Error(message);
228243
}
229244
})
245+
.then(() => {
246+
// Populate the file system cache with the virtual module.
247+
this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal);
248+
})
230249
.then(() => {
231250
// Process the lazy routes
232-
this._lazyRoutes =
233-
this._processNgModule(this._entryModule, null)
234-
.map(module => ModuleRoute.fromString(module))
235-
.reduce((lazyRoutes: any, module: ModuleRoute) => {
236-
lazyRoutes[`${module.path}.ngfactory`] = path.join(
237-
this.genDir, module.path + '.ngfactory.ts');
238-
return lazyRoutes;
239-
}, {});
251+
this._lazyRoutes = {};
252+
const allLazyRoutes = this._processNgModule(this._entryModule, null);
253+
Object.keys(allLazyRoutes)
254+
.forEach(k => {
255+
const lazyRoute = allLazyRoutes[k];
256+
this._lazyRoutes[k + '.ngfactory'] = lazyRoute.moduleAbsolutePath + '.ngfactory.ts';
257+
});
240258
})
241-
.then(() => cb(), (err: any) => cb(err));
259+
.then(() => cb(), (err: any) => { cb(err); });
242260
}
243261

244-
private _resolveModule(module: ModuleRoute, containingFile: string) {
262+
private _resolveModulePath(module: ModuleRoute, containingFile: string) {
245263
if (module.path.startsWith('.')) {
246264
return path.join(path.dirname(containingFile), module.path);
247265
}
248266
return module.path;
249267
}
250268

251-
private _processNgModule(module: ModuleRoute, containingFile: string | null): string[] {
269+
private _processNgModule(module: ModuleRoute, containingFile: string | null): LazyRouteMap {
252270
const modulePath = containingFile ? module.path : ('./' + path.basename(module.path));
253271
if (containingFile === null) {
254272
containingFile = module.path + '.ts';
255273
}
274+
const relativeModulePath = this._resolveModulePath(module, containingFile);
256275

257-
const resolvedModulePath = this._resolveModule(module, containingFile);
258276
const staticSymbol = this._reflectorHost
259277
.findDeclaration(modulePath, module.className, containingFile);
260278
const entryNgModuleMetadata = this.getNgModuleMetadata(staticSymbol);
261-
const loadChildren = this.extractLoadChildren(entryNgModuleMetadata);
262-
const result = loadChildren.map(route => {
263-
return this._resolveModule(new ModuleRoute(route), resolvedModulePath);
264-
});
279+
const loadChildrenRoute: LazyRoute[] = this.extractLoadChildren(entryNgModuleMetadata)
280+
.map(route => {
281+
const mr = ModuleRoute.fromString(route);
282+
const relativePath = this._resolveModulePath(mr, relativeModulePath);
283+
const absolutePath = path.join(this.genDir, relativePath);
284+
return {
285+
moduleRoute: mr,
286+
moduleRelativePath: relativePath,
287+
moduleAbsolutePath: absolutePath
288+
};
289+
});
290+
const resultMap: LazyRouteMap = loadChildrenRoute
291+
.reduce((acc: LazyRouteMap, curr: LazyRoute) => {
292+
const key = curr.moduleRoute.path;
293+
if (acc[key]) {
294+
if (acc[key].moduleAbsolutePath != curr.moduleAbsolutePath) {
295+
throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` +
296+
'loadChildren, but they point to different modules. Webpack cannot distinguish ' +
297+
'between the two based on context and would fail to load the proper one.');
298+
}
299+
} else {
300+
acc[key] = curr;
301+
}
302+
return acc;
303+
}, {});
265304

266305
// Also concatenate every child of child modules.
267-
for (const route of loadChildren) {
268-
const childModule = ModuleRoute.fromString(route);
269-
const children = this._processNgModule(childModule, resolvedModulePath + '.ts');
270-
result.push(...children);
306+
for (const lazyRoute of loadChildrenRoute) {
307+
const mr = lazyRoute.moduleRoute;
308+
const children = this._processNgModule(mr, relativeModulePath);
309+
Object.keys(children).forEach(p => {
310+
const child = children[p];
311+
const key = child.moduleRoute.path;
312+
if (resultMap[key]) {
313+
if (resultMap[key].moduleAbsolutePath != child.moduleAbsolutePath) {
314+
throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` +
315+
'loadChildren, but they point to different modules. Webpack cannot distinguish ' +
316+
'between the two based on context and would fail to load the proper one.');
317+
}
318+
} else {
319+
resultMap[key] = child;
320+
}
321+
});
271322
}
272-
return result;
323+
return resultMap;
273324
}
274325

275326
private getNgModuleMetadata(staticSymbol: ngCompiler.StaticSymbol) {
@@ -281,10 +332,23 @@ export class AotPlugin {
281332
}
282333

283334
private extractLoadChildren(ngModuleDecorator: any): any[] {
284-
const routes = ngModuleDecorator.imports.reduce((mem: any[], m: any) => {
335+
const routes = (ngModuleDecorator.imports || []).reduce((mem: any[], m: any) => {
285336
return mem.concat(this.collectRoutes(m.providers));
286337
}, this.collectRoutes(ngModuleDecorator.providers));
287-
return this.collectLoadChildren(routes);
338+
return this.collectLoadChildren(routes)
339+
.concat((ngModuleDecorator.imports || [])
340+
// Also recursively extractLoadChildren of modules we import.
341+
.map((staticSymbol: any) => {
342+
if (staticSymbol instanceof StaticSymbol) {
343+
const entryNgModuleMetadata = this.getNgModuleMetadata(staticSymbol);
344+
return this.extractLoadChildren(entryNgModuleMetadata);
345+
} else {
346+
return [];
347+
}
348+
})
349+
// Poor man's flat map.
350+
.reduce((acc: any[], i: any) => acc.concat(i), []))
351+
.filter(x => !!x);
288352
}
289353

290354
private collectRoutes(providers: any[]): any[] {

tests/e2e_runner.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ testsToRun.reduce((previous, relativeName) => {
9595
return Promise.resolve()
9696
.then(() => printHeader(currentFileName))
9797
.then(() => fn(argv, () => clean = false))
98+
.then(() => console.log(' ----'))
9899
.then(() => {
99100
// Only clean after a real test, not a setup step. Also skip cleaning if the test
100101
// requested an exception.
@@ -104,7 +105,9 @@ testsToRun.reduce((previous, relativeName) => {
104105
})
105106
.then(() => printFooter(currentFileName, start),
106107
(err) => {
107-
printFooter(currentFileName, start); throw err;
108+
printFooter(currentFileName, start);
109+
console.error(err);
110+
throw err;
108111
});
109112
});
110113
}, Promise.resolve())

0 commit comments

Comments
 (0)