From d820065d464c8d04650acd1398c660b989876f7b Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Tue, 16 Oct 2018 07:58:11 +0200 Subject: [PATCH 1/2] fix(@ngtools/webpack): emit lazy routes errors on rebuilds At the moment lazy route errors are only emitted in the initial builds because in following builds we are only processed lazy routes that are declared in the changed files. At the moment, we cannot cache the previously resolved routes as there is no way to track in which file the lazy route was declared so that we can bust the lazy route if it was removed. Closes #12236 --- .../test/browser/lazy-module_spec_large.ts | 49 ++++++++- .../webpack/src/angular_compiler_plugin.ts | 35 ++---- packages/ngtools/webpack/src/lazy_routes.ts | 100 ------------------ .../transformers/export_lazy_module_map.ts | 5 +- 4 files changed, 58 insertions(+), 131 deletions(-) delete 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 c334fd998178..c6dbf303f186 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,50 @@ 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/ngtools/webpack/src/angular_compiler_plugin.ts b/packages/ngtools/webpack/src/angular_compiler_plugin.ts index 472603db1043..064601a8d85f 100644 --- a/packages/ngtools/webpack/src/angular_compiler_plugin.ts +++ b/packages/ngtools/webpack/src/angular_compiler_plugin.ts @@ -40,10 +40,10 @@ import { time, timeEnd } from './benchmark'; import { WebpackCompilerHost, workaroundResolve } from './compiler_host'; import { resolveEntryModuleFromMain } from './entry_resolver'; import { 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, @@ -418,22 +418,6 @@ export class AngularCompilerPlugin { } } - private _findLazyRoutesInAst(changedFilePaths: string[]): LazyRouteMap { - time('AngularCompilerPlugin._findLazyRoutesInAst'); - const result: LazyRouteMap = Object.create(null); - 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 lazyRoutes: LazyRoute[]; if (this._JitMode) { @@ -876,17 +860,12 @@ export class AngularCompilerPlugin { // 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. - if (this._firstRun || !this._JitMode) { - this._processLazyRoutes(this._listLazyRoutesFromProgram()); - } else { - const changedTsFiles = this._getChangedTsFiles(); - if (changedTsFiles.length > 0) { - this._processLazyRoutes(this._findLazyRoutesInAst(changedTsFiles)); - } - } - if (this._options.additionalLazyModules) { - this._processLazyRoutes(this._options.additionalLazyModules); - } + const lazyRouteMap: LazyRouteMap = { + ... (this._entryModule || !this._JitMode ? this._listLazyRoutesFromProgram() : {}), + ...this._options.additionalLazyModules, + }; + + this._processLazyRoutes(lazyRouteMap); // Emit and report errors. diff --git a/packages/ngtools/webpack/src/lazy_routes.ts b/packages/ngtools/webpack/src/lazy_routes.ts deleted file mode 100644 index dc32ebcea8c5..000000000000 --- a/packages/ngtools/webpack/src/lazy_routes.ts +++ /dev/null @@ -1,100 +0,0 @@ -/** - * @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 9cfc9be42db7..02b02058e66e 100644 --- a/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts +++ b/packages/ngtools/webpack/src/transformers/export_lazy_module_map.ts @@ -7,11 +7,14 @@ */ 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 f634be0fa62cef3d6fa0ef5f926b4c3a91798184 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Tue, 16 Oct 2018 08:22:00 +0200 Subject: [PATCH 2/2] test: add test for compilation errors in watch mode Closes #12311 --- .../test/browser/rebuild_spec_large.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) 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 6ee1df2fdd76..9d9523965cda 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,6 +198,33 @@ 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`.