Skip to content

Commit 51e87ae

Browse files
committed
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 #2735 Fixes #2900
1 parent cabc9dc commit 51e87ae

File tree

8 files changed

+109
-38
lines changed

8 files changed

+109
-38
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

+96-28
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,8 +26,24 @@ 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 {
29-
constructor(public readonly path: string, public readonly className: string = null) {}
42+
constructor(public readonly path: string, public readonly className: string = null) {
43+
if (arguments.length == 1) {
44+
[this.path, this.className] = path.split('#');
45+
}
46+
}
3047

3148
toString() {
3249
return `${this.path}#${this.className}`;
@@ -57,6 +74,7 @@ export class AotPlugin {
5774

5875
private _typeCheck: boolean = true;
5976
private _basePath: string;
77+
private _genDir: string;
6078

6179

6280
constructor(options: AotPluginOptions) {
@@ -68,7 +86,7 @@ export class AotPlugin {
6886
get compilerOptions() { return this._compilerOptions; }
6987
get done() { return this._donePromise; }
7088
get entryModule() { return this._entryModule; }
71-
get genDir() { return this._basePath; }
89+
get genDir() { return this._genDir; }
7290
get program() { return this._program; }
7391
get typeCheck() { return this._typeCheck; }
7492

@@ -113,6 +131,7 @@ export class AotPlugin {
113131
genDir
114132
});
115133
this._basePath = basePath;
134+
this._genDir = genDir;
116135

117136
if (options.hasOwnProperty('typeChecking')) {
118137
this._typeCheck = options.typeChecking;
@@ -168,9 +187,9 @@ export class AotPlugin {
168187

169188
// Virtual file system.
170189
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) {
190+
if (request.request.match(/\.ts$/)) {
191+
this.done.then(() => cb());
192+
} else {
174193
cb();
175194
}
176195
});
@@ -227,49 +246,85 @@ export class AotPlugin {
227246
throw new Error(message);
228247
}
229248
})
249+
.then(() => {
250+
// Populate the file system cache with the virtual module.
251+
this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal);
252+
})
230253
.then(() => {
231254
// 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-
}, {});
255+
this._lazyRoutes = {};
256+
const allLazyRoutes = this._processNgModule(this._entryModule, null);
257+
Object.keys(allLazyRoutes)
258+
.forEach(k => {
259+
const lazyRoute = allLazyRoutes[k];
260+
this._lazyRoutes[k + '.ngfactory'] = lazyRoute.moduleAbsolutePath + '.ngfactory.ts';
261+
});
240262
})
241-
.then(() => cb(), (err: any) => cb(err));
263+
.then(() => cb(), (err: any) => { cb(err); });
242264
}
243265

244-
private _resolveModule(module: ModuleRoute, containingFile: string) {
266+
private _resolveModulePath(module: ModuleRoute, containingFile: string) {
245267
if (module.path.startsWith('.')) {
246268
return path.join(path.dirname(containingFile), module.path);
247269
}
248270
return module.path;
249271
}
250272

251-
private _processNgModule(module: ModuleRoute, containingFile: string | null): string[] {
273+
private _processNgModule(module: ModuleRoute, containingFile: string | null): LazyRouteMap {
252274
const modulePath = containingFile ? module.path : ('./' + path.basename(module.path));
253275
if (containingFile === null) {
254276
containingFile = module.path + '.ts';
255277
}
278+
const relativeModulePath = this._resolveModulePath(module, containingFile);
256279

257-
const resolvedModulePath = this._resolveModule(module, containingFile);
258280
const staticSymbol = this._reflectorHost
259281
.findDeclaration(modulePath, module.className, containingFile);
260282
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-
});
283+
const loadChildrenRoute: LazyRoute[] = this.extractLoadChildren(entryNgModuleMetadata)
284+
.map(route => {
285+
const mr = ModuleRoute.fromString(route);
286+
const relativePath = this._resolveModulePath(mr, relativeModulePath);
287+
const absolutePath = path.join(this.genDir, relativePath);
288+
return {
289+
moduleRoute: mr,
290+
moduleRelativePath: relativePath,
291+
moduleAbsolutePath: absolutePath
292+
};
293+
});
294+
const resultMap: LazyRouteMap = loadChildrenRoute
295+
.reduce((acc: LazyRouteMap, curr: LazyRoute) => {
296+
const key = curr.moduleRoute.path;
297+
if (acc[key]) {
298+
if (acc[key].moduleAbsolutePath != curr.moduleAbsolutePath) {
299+
throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` +
300+
'loadChildren, but they point to different modules. Webpack cannot distinguish ' +
301+
'between the two based on context and would fail to load the proper one.');
302+
}
303+
} else {
304+
acc[key] = curr;
305+
}
306+
return acc;
307+
}, {});
265308

266309
// 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);
310+
for (const lazyRoute of loadChildrenRoute) {
311+
const mr = lazyRoute.moduleRoute;
312+
const children = this._processNgModule(mr, relativeModulePath);
313+
Object.keys(children).forEach(p => {
314+
const child = children[p];
315+
const key = child.moduleRoute.path;
316+
if (resultMap[key]) {
317+
if (resultMap[key].moduleAbsolutePath != child.moduleAbsolutePath) {
318+
throw new Error(`Duplicated path in loadChildren detected: "${key}" is used in 2 ` +
319+
'loadChildren, but they point to different modules. Webpack cannot distinguish ' +
320+
'between the two based on context and would fail to load the proper one.');
321+
}
322+
} else {
323+
resultMap[key] = child;
324+
}
325+
});
271326
}
272-
return result;
327+
return resultMap;
273328
}
274329

275330
private getNgModuleMetadata(staticSymbol: ngCompiler.StaticSymbol) {
@@ -281,10 +336,23 @@ export class AotPlugin {
281336
}
282337

283338
private extractLoadChildren(ngModuleDecorator: any): any[] {
284-
const routes = ngModuleDecorator.imports.reduce((mem: any[], m: any) => {
339+
const routes = (ngModuleDecorator.imports || []).reduce((mem: any[], m: any) => {
285340
return mem.concat(this.collectRoutes(m.providers));
286341
}, this.collectRoutes(ngModuleDecorator.providers));
287-
return this.collectLoadChildren(routes);
342+
return this.collectLoadChildren(routes)
343+
.concat((ngModuleDecorator.imports || [])
344+
// Also recursively extractLoadChildren of modules we import.
345+
.map((staticSymbol: any) => {
346+
if (staticSymbol instanceof StaticSymbol) {
347+
const entryNgModuleMetadata = this.getNgModuleMetadata(staticSymbol);
348+
return this.extractLoadChildren(entryNgModuleMetadata);
349+
} else {
350+
return [];
351+
}
352+
})
353+
// Poor man's flat map.
354+
.reduce((acc: any[], i: any) => acc.concat(i), []))
355+
.filter(x => !!x);
288356
}
289357

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

tests/e2e/tests/build/styles/styles-array.ts

+3-4
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,10 @@ export default function() {
4747
.then(() => expectFileToMatch('dist/styles.bundle.js', /.upper.*.lower.*background.*#def/))
4848

4949
.then(() => ng('build', '--prod'))
50-
.then(() => new Promise<string>(() =>
51-
glob.sync('dist/styles.*.bundle.css').find(file => !!file)))
50+
.then(() => glob.sync('dist/styles.*.bundle.css').find(file => !!file))
5251
.then((styles) =>
53-
expectFileToMatch(styles, 'body { background-color: blue; }')
54-
.then(() => expectFileToMatch(styles, 'p { background-color: red; }')
52+
expectFileToMatch(styles, /body\s*\{\s*background-color:\s*blue\s*\}/)
53+
.then(() => expectFileToMatch(styles, /p\s*\{\s*background-color:\s*red\s*\}/)
5554
.then(() => expectFileToMatch(styles, /.outer.*.inner.*background:\s*#[fF]+/))
5655
.then(() => expectFileToMatch(styles, /.upper.*.lower.*background.*#def/)))
5756
);

tests/e2e/tests/generate/component/component-path-case.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export default function() {
88
const componentDir = join(rootDir.toLowerCase(), 'test-component');
99
createDir(rootDir);
1010

11-
return ng('generate', 'component', 'Upper-Dir', 'test-component')
11+
return ng('generate', 'component', 'Upper-Dir/test-component')
1212
.then(() => expectFileToExist(componentDir))
1313
.then(() => expectFileToExist(join(componentDir, 'test-component.component.ts')))
1414
.then(() => expectFileToExist(join(componentDir, 'test-component.component.spec.ts')))

tests/e2e_runner.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ if (testsToRun.length == allTests.length) {
7474
}
7575

7676
testsToRun.reduce((previous, relativeName) => {
77+
console.log(relativeName);
7778
// Make sure this is a windows compatible path.
7879
let absoluteName = path.join(e2eRoot, relativeName);
7980
if (/^win/.test(process.platform)) {
@@ -95,6 +96,7 @@ testsToRun.reduce((previous, relativeName) => {
9596
return Promise.resolve()
9697
.then(() => printHeader(currentFileName))
9798
.then(() => fn(argv, () => clean = false))
99+
.then(() => console.log(' ----'))
98100
.then(() => {
99101
// Only clean after a real test, not a setup step. Also skip cleaning if the test
100102
// requested an exception.
@@ -104,7 +106,9 @@ testsToRun.reduce((previous, relativeName) => {
104106
})
105107
.then(() => printFooter(currentFileName, start),
106108
(err) => {
107-
printFooter(currentFileName, start); throw err;
109+
printFooter(currentFileName, start);
110+
console.error(err);
111+
throw err;
108112
});
109113
});
110114
}, Promise.resolve())

0 commit comments

Comments
 (0)