From e20ba14da61e52edf388e84a37a852bb659395a5 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 27 Dec 2018 11:43:28 +0100 Subject: [PATCH 1/2] revert: fix(@ngtools/webpack): emit lazy routes errors on rebuilds This reverts commit edb84b340ff996df2ca6a2aaf765304cc64fef3e The team has decided to bring back the faster but potentially less accurate method of detecting lazy routes upon JIT rebuilds (first builds will always use the more complete Angular compiler method). Applications that do not have lazy routes within libraries and that only use direct string literals with loadChildren should not be affected by the potential of less accurate detection. Note that the function overload of loadChildren also does not apply to this situation. For those projects where correctness of lazy route detection outweighs rebuild speed, please consider using AOT mode for development. AOT mode will also provide a full set of template errors as well which JIT mode is not capable of doing. Fixes #13102 --- .../test/browser/lazy-module_spec_large.ts | 49 +-------- .../test/browser/rebuild_spec_large.ts | 27 ----- .../webpack/src/angular_compiler_plugin.ts | 45 +++++++- packages/ngtools/webpack/src/lazy_routes.ts | 100 ++++++++++++++++++ .../transformers/export_lazy_module_map.ts | 5 +- 5 files changed, 144 insertions(+), 82 deletions(-) create mode 100644 packages/ngtools/webpack/src/lazy_routes.ts diff --git a/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts index 001751bb86d2..2177de386354 100644 --- a/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts +++ b/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import { DefaultTimeout, TestLogger, runTargetSpec } from '@angular-devkit/architect/testing'; +import { DefaultTimeout, runTargetSpec } from '@angular-devkit/architect/testing'; import { join, normalize } from '@angular-devkit/core'; -import { take, tap } from 'rxjs/operators'; +import { tap } from 'rxjs/operators'; import { BrowserBuilderSchema } from '../../src'; import { browserTargetSpec, host } from '../utils'; @@ -70,7 +70,6 @@ export const lazyModuleImport: { [path: string]: string } = { `, }; -// tslint:disable-next-line:no-big-function describe('Browser Builder lazy modules', () => { const outputPath = normalize('dist'); @@ -90,50 +89,6 @@ describe('Browser Builder lazy modules', () => { ).toPromise().then(done, done.fail); }); - it('should show error when lazy route is invalid on watch mode AOT', (done) => { - host.writeMultipleFiles(lazyModuleFiles); - host.writeMultipleFiles(lazyModuleImport); - host.replaceInFile( - 'src/app/app.module.ts', - 'lazy.module#LazyModule', - 'invalid.module#LazyModule', - ); - - const logger = new TestLogger('rebuild-lazy-errors'); - const overrides = { watch: true, aot: true }; - runTargetSpec(host, browserTargetSpec, overrides, DefaultTimeout, logger).pipe( - tap((buildEvent) => expect(buildEvent.success).toBe(false)), - tap(() => { - expect(logger.includes('Could not resolve module')).toBe(true); - logger.clear(); - host.appendToFile('src/main.ts', ' '); - }), - take(2), - ).toPromise().then(done, done.fail); - }); - - it('should show error when lazy route is invalid on watch mode JIT', (done) => { - host.writeMultipleFiles(lazyModuleFiles); - host.writeMultipleFiles(lazyModuleImport); - host.replaceInFile( - 'src/app/app.module.ts', - 'lazy.module#LazyModule', - 'invalid.module#LazyModule', - ); - - const logger = new TestLogger('rebuild-lazy-errors'); - const overrides = { watch: true, aot: false }; - runTargetSpec(host, browserTargetSpec, overrides, DefaultTimeout, logger).pipe( - tap((buildEvent) => expect(buildEvent.success).toBe(false)), - tap(() => { - expect(logger.includes('Could not resolve module')).toBe(true); - logger.clear(); - host.appendToFile('src/main.ts', ' '); - }), - take(2), - ).toPromise().then(done, done.fail); - }); - it('supports lazy bundle for lazy routes with AOT', (done) => { host.writeMultipleFiles(lazyModuleFiles); host.writeMultipleFiles(lazyModuleImport); diff --git a/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts index 778b3b687376..acd6354a4aca 100644 --- a/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts +++ b/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts @@ -198,33 +198,6 @@ describe('Browser Builder rebuilds', () => { ).toPromise().then(done, done.fail); }); - it('rebuilds shows error', (done) => { - host.replaceInFile('./src/app/app.component.ts', 'AppComponent', 'AppComponentZ'); - - const overrides = { watch: true, aot: false }; - let buildCount = 1; - const logger = new TestLogger('rebuild-errors'); - - runTargetSpec(host, browserTargetSpec, overrides, DefaultTimeout * 3, logger).pipe( - tap((buildEvent) => { - switch (buildCount) { - case 1: - expect(buildEvent.success).toBe(false); - expect(logger.includes('AppComponent cannot be used as an entry component')).toBe(true); - logger.clear(); - - host.replaceInFile('./src/app/app.component.ts', 'AppComponentZ', 'AppComponent'); - break; - - default: - expect(buildEvent.success).toBe(true); - break; - } - buildCount ++; - }), - take(2), - ).toPromise().then(done, done.fail); - }); it('rebuilds after errors in AOT', (done) => { // Save the original contents of `./src/app/app.component.ts`. diff --git a/packages/ngtools/webpack/src/angular_compiler_plugin.ts b/packages/ngtools/webpack/src/angular_compiler_plugin.ts index ae315372ed6e..d30cb42abcca 100644 --- a/packages/ngtools/webpack/src/angular_compiler_plugin.ts +++ b/packages/ngtools/webpack/src/angular_compiler_plugin.ts @@ -39,10 +39,10 @@ import { time, timeEnd } from './benchmark'; import { WebpackCompilerHost, workaroundResolve } from './compiler_host'; import { resolveEntryModuleFromMain } from './entry_resolver'; import { DiagnosticMode, gatherDiagnostics, hasErrors } from './gather_diagnostics'; +import { LazyRouteMap, findLazyRoutes } from './lazy_routes'; import { TypeScriptPathsPlugin } from './paths-plugin'; import { WebpackResourceLoader } from './resource_loader'; import { - LazyRouteMap, exportLazyModuleMap, exportNgFactory, findResources, @@ -135,7 +135,7 @@ export class AngularCompilerPlugin { private _moduleResolutionCache: ts.ModuleResolutionCache; private _resourceLoader?: WebpackResourceLoader; // Contains `moduleImportPath#exportName` => `fullModulePath`. - private _lazyRoutes: LazyRouteMap = Object.create(null); + private _lazyRoutes: LazyRouteMap = {}; private _tsConfigPath: string; private _entryModule: string | null; private _mainPath: string | undefined; @@ -421,6 +421,22 @@ export class AngularCompilerPlugin { } } + private _findLazyRoutesInAst(changedFilePaths: string[]): LazyRouteMap { + time('AngularCompilerPlugin._findLazyRoutesInAst'); + const result: LazyRouteMap = {}; + for (const filePath of changedFilePaths) { + const fileLazyRoutes = findLazyRoutes(filePath, this._compilerHost, undefined, + this._compilerOptions); + for (const routeKey of Object.keys(fileLazyRoutes)) { + const route = fileLazyRoutes[routeKey]; + result[routeKey] = route; + } + } + timeEnd('AngularCompilerPlugin._findLazyRoutesInAst'); + + return result; + } + private _listLazyRoutesFromProgram(): LazyRouteMap { let entryRoute: string | undefined; let ngProgram: Program; @@ -876,6 +892,12 @@ export class AngularCompilerPlugin { } } + private _getChangedTsFiles() { + return this._getChangedCompilationFiles() + .filter(k => (k.endsWith('.ts') || k.endsWith('.tsx')) && !k.endsWith('.d.ts')) + .filter(k => this._compilerHost.fileExists(k)); + } + private async _update() { time('AngularCompilerPlugin._update'); // We only want to update on TS and template changes, but all kinds of files are on this @@ -890,11 +912,26 @@ export class AngularCompilerPlugin { // Make a new program and load the Angular structure. await this._createOrUpdateProgram(); + // Try to find lazy routes if we have an entry module. + // We need to run the `listLazyRoutes` the first time because it also navigates libraries + // and other things that we might miss using the (faster) findLazyRoutesInAst. + // Lazy routes modules will be read with compilerHost and added to the changed files. + let lazyRouteMap: LazyRouteMap = {}; + if (!this._JitMode || this._firstRun) { + lazyRouteMap = this._listLazyRoutesFromProgram(); + } else { + const changedTsFiles = this._getChangedTsFiles(); + if (changedTsFiles.length > 0) { + lazyRouteMap = this._findLazyRoutesInAst(changedTsFiles); + } + } + // Find lazy routes - const lazyRouteMap: LazyRouteMap = { - ...this._listLazyRoutesFromProgram(), + lazyRouteMap = { + ...lazyRouteMap, ...this._options.additionalLazyModules, }; + this._processLazyRoutes(lazyRouteMap); // Emit files. diff --git a/packages/ngtools/webpack/src/lazy_routes.ts b/packages/ngtools/webpack/src/lazy_routes.ts new file mode 100644 index 000000000000..dc32ebcea8c5 --- /dev/null +++ b/packages/ngtools/webpack/src/lazy_routes.ts @@ -0,0 +1,100 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import { dirname, join } from 'path'; +import * as ts from 'typescript'; +import { findAstNodes, resolve } from './refactor'; + + +function _getContentOfKeyLiteral(_source: ts.SourceFile, node: ts.Node): string | null { + if (node.kind == ts.SyntaxKind.Identifier) { + return (node as ts.Identifier).text; + } else if (node.kind == ts.SyntaxKind.StringLiteral) { + return (node as ts.StringLiteral).text; + } else { + return null; + } +} + + +export interface LazyRouteMap { + [path: string]: string; +} + + +export function findLazyRoutes( + filePath: string, + host: ts.CompilerHost, + program?: ts.Program, + compilerOptions?: ts.CompilerOptions, +): LazyRouteMap { + if (!compilerOptions) { + if (!program) { + throw new Error('Must pass either program or compilerOptions to findLazyRoutes.'); + } + compilerOptions = program.getCompilerOptions(); + } + const fileName = resolve(filePath, host, compilerOptions).replace(/\\/g, '/'); + let sourceFile: ts.SourceFile | undefined; + if (program) { + sourceFile = program.getSourceFile(fileName); + } + + if (!sourceFile) { + const content = host.readFile(fileName); + if (content) { + sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.Latest, true); + } + } + + if (!sourceFile) { + throw new Error(`Source file not found: '${fileName}'.`); + } + const sf: ts.SourceFile = sourceFile; + + return findAstNodes(null, sourceFile, ts.SyntaxKind.ObjectLiteralExpression, true) + // Get all their property assignments. + .map((node: ts.ObjectLiteralExpression) => { + return findAstNodes(node, sf, ts.SyntaxKind.PropertyAssignment, false); + }) + // Take all `loadChildren` elements. + .reduce((acc: ts.PropertyAssignment[], props: ts.PropertyAssignment[]) => { + return acc.concat(props.filter(literal => { + return _getContentOfKeyLiteral(sf, literal.name) == 'loadChildren'; + })); + }, []) + // Get only string values. + .filter((node: ts.PropertyAssignment) => node.initializer.kind == ts.SyntaxKind.StringLiteral) + // Get the string value. + .map((node: ts.PropertyAssignment) => (node.initializer as ts.StringLiteral).text) + // Map those to either [path, absoluteModulePath], or [path, null] if the module pointing to + // does not exist. + .map((routePath: string) => { + const moduleName = routePath.split('#')[0]; + const compOptions = (program && program.getCompilerOptions()) || compilerOptions || {}; + const resolvedModuleName: ts.ResolvedModuleWithFailedLookupLocations = moduleName[0] == '.' + ? ({ + resolvedModule: { resolvedFileName: join(dirname(filePath), moduleName) + '.ts' }, + } as ts.ResolvedModuleWithFailedLookupLocations) + : ts.resolveModuleName(moduleName, filePath, compOptions, host); + if (resolvedModuleName.resolvedModule + && resolvedModuleName.resolvedModule.resolvedFileName + && host.fileExists(resolvedModuleName.resolvedModule.resolvedFileName)) { + return [routePath, resolvedModuleName.resolvedModule.resolvedFileName]; + } else { + return [routePath, null]; + } + }) + // Reduce to the LazyRouteMap map. + .reduce((acc: LazyRouteMap, [routePath, resolvedModuleName]: [string, string | null]) => { + if (resolvedModuleName) { + acc[routePath] = resolvedModuleName; + } + + return acc; + }, {}); +} diff --git a/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts b/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts index 02b02058e66e..9cfc9be42db7 100644 --- a/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts +++ b/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts @@ -7,14 +7,11 @@ */ import * as path from 'path'; import * as ts from 'typescript'; +import { LazyRouteMap } from '../lazy_routes'; import { getFirstNode, getLastNode } from './ast_helpers'; import { AddNodeOperation, StandardTransform, TransformOperation } from './interfaces'; import { makeTransform } from './make_transform'; -export interface LazyRouteMap { - [path: string]: string; -} - export function exportLazyModuleMap( shouldTransform: (fileName: string) => boolean, lazyRoutesCb: () => LazyRouteMap, From 2b408f245ec8ce9bbff4ab82441d5fcf1b96c5e2 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 27 Dec 2018 12:52:02 +0100 Subject: [PATCH 2/2] test: add test for invalid lazy routes in AOT --- .../test/browser/lazy-module_spec_large.ts | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts index 2177de386354..121aaf43f222 100644 --- a/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts +++ b/packages/angular_devkit/build_angular/test/browser/lazy-module_spec_large.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import { DefaultTimeout, runTargetSpec } from '@angular-devkit/architect/testing'; +import { DefaultTimeout, TestLogger, runTargetSpec } from '@angular-devkit/architect/testing'; import { join, normalize } from '@angular-devkit/core'; -import { tap } from 'rxjs/operators'; +import { take, tap } from 'rxjs/operators'; import { BrowserBuilderSchema } from '../../src'; import { browserTargetSpec, host } from '../utils'; @@ -70,6 +70,7 @@ export const lazyModuleImport: { [path: string]: string } = { `, }; +// tslint:disable-next-line:no-big-function describe('Browser Builder lazy modules', () => { const outputPath = normalize('dist'); @@ -89,6 +90,28 @@ describe('Browser Builder lazy modules', () => { ).toPromise().then(done, done.fail); }); + it('should show error when lazy route is invalid on watch mode AOT', (done) => { + host.writeMultipleFiles(lazyModuleFiles); + host.writeMultipleFiles(lazyModuleImport); + host.replaceInFile( + 'src/app/app.module.ts', + 'lazy.module#LazyModule', + 'invalid.module#LazyModule', + ); + + const logger = new TestLogger('rebuild-lazy-errors'); + const overrides = { watch: true, aot: true }; + runTargetSpec(host, browserTargetSpec, overrides, DefaultTimeout, logger).pipe( + tap((buildEvent) => expect(buildEvent.success).toBe(false)), + tap(() => { + expect(logger.includes('Could not resolve module')).toBe(true); + logger.clear(); + host.appendToFile('src/main.ts', ' '); + }), + take(2), + ).toPromise().then(done, done.fail); + }); + it('supports lazy bundle for lazy routes with AOT', (done) => { host.writeMultipleFiles(lazyModuleFiles); host.writeMultipleFiles(lazyModuleImport);