Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(aot): Use the proper path when statically analyzing lazy routes.
Browse files Browse the repository at this point in the history
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.

BREAKING CHANGES: Using relative paths might lead to path clashing. We
now properly output an error in this case.

Fixes angular#2452
Fixes angular#2900
hansl committed Nov 3, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent cabc9dc commit 3739374
Showing 7 changed files with 105 additions and 36 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -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",
2 changes: 1 addition & 1 deletion packages/angular-cli/package.json
Original file line number Diff line number Diff line change
@@ -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",
2 changes: 1 addition & 1 deletion packages/webpack/src/loader.ts
Original file line number Diff line number Diff line change
@@ -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 => {
120 changes: 93 additions & 27 deletions packages/webpack/src/plugin.ts
Original file line number Diff line number Diff line change
@@ -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}`;
@@ -168,9 +185,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 +244,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 +334,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[] {
7 changes: 3 additions & 4 deletions tests/e2e/tests/build/styles/styles-array.ts
Original file line number Diff line number Diff line change
@@ -47,11 +47,10 @@ export default function() {
.then(() => expectFileToMatch('dist/styles.bundle.js', /.upper.*.lower.*background.*#def/))

.then(() => ng('build', '--prod'))
.then(() => new Promise<string>(() =>
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/)))
);
2 changes: 1 addition & 1 deletion tests/e2e/tests/generate/component/component-path-case.ts
Original file line number Diff line number Diff line change
@@ -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')))
6 changes: 5 additions & 1 deletion tests/e2e_runner.js
Original file line number Diff line number Diff line change
@@ -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())

0 comments on commit 3739374

Please sign in to comment.