perf(@ngtools/webpack): improve rebuild performance (angular#4188)

hansl · MRHarrison · commit e82feab85fef · 2017-02-08T16:05:44.000-08:00
We need to run full static analysis on the first build to discover routes
in node_modules, but in rebuilding the app we just need to look for the
changed files.

This introduces a subtle bug though; if the module structure changes, we
might be missing lazy routes if those are in modules imported but
weren't in the original structure.

This is, for now, considered "okay" as it's a relatively rare case. We
should probably output a warning though.
diff --git a/packages/@ngtools/webpack/src/compiler_host.ts b/packages/@ngtools/webpack/src/compiler_host.ts
@@ -95,7 +95,9 @@ export class WebpackCompilerHost implements ts.CompilerHost {
   private _delegate: ts.CompilerHost;
   private _files: {[path: string]: VirtualFileStats} = Object.create(null);
   private _directories: {[path: string]: VirtualDirStats} = Object.create(null);
-  private _changed = false;
+
+  private _changedFiles: {[path: string]: boolean} = Object.create(null);
+  private _changedDirs: {[path: string]: boolean} = Object.create(null);
 
   private _basePath: string;
   private _setParentNodes: boolean;
@@ -129,10 +131,15 @@ export class WebpackCompilerHost implements ts.CompilerHost {
     let p = dirname(fileName);
     while (p && !this._directories[p]) {
       this._directories[p] = new VirtualDirStats(p);
+      this._changedDirs[p] = true;
       p = dirname(p);
     }
 
-    this._changed = true;
+    this._changedFiles[fileName] = true;
+  }
+
+  get dirty() {
+    return Object.keys(this._changedFiles).length > 0;
   }
 
   enableCaching() {
@@ -141,21 +148,26 @@ export class WebpackCompilerHost implements ts.CompilerHost {
 
   populateWebpackResolver(resolver: any) {
     const fs = resolver.fileSystem;
-    if (!this._changed) {
+    if (!this.dirty) {
       return;
     }
 
     const isWindows = process.platform.startsWith('win');
-    for (const fileName of Object.keys(this._files)) {
+    for (const fileName of this.getChangedFilePaths()) {
       const stats = this._files[fileName];
       if (stats) {
         // If we're on windows, we need to populate with the proper path separator.
         const path = isWindows ? fileName.replace(/\//g, '\\') : fileName;
         fs._statStorage.data[path] = [null, stats];
         fs._readFileStorage.data[path] = [null, stats.content];
+      } else {
+        // Support removing files as well.
+        const path = isWindows ? fileName.replace(/\//g, '\\') : fileName;
+        fs._statStorage.data[path] = [new Error(), null];
+        fs._readFileStorage.data[path] = [new Error(), null];
       }
     }
-    for (const dirName of Object.keys(this._directories)) {
+    for (const dirName of Object.keys(this._changedDirs)) {
       const stats = this._directories[dirName];
       const dirs = this.getDirectories(dirName);
       const files = this.getFiles(dirName);
@@ -164,12 +176,19 @@ export class WebpackCompilerHost implements ts.CompilerHost {
       fs._statStorage.data[path] = [null, stats];
       fs._readdirStorage.data[path] = [null, files.concat(dirs)];
     }
+  }
 
-    this._changed = false;
+  resetChangedFileTracker() {
+    this._changedFiles = Object.create(null);
+    this._changedDirs = Object.create(null);
+  }
+  getChangedFilePaths(): string[] {
+    return Object.keys(this._changedFiles);
   }
 
   invalidate(fileName: string): void {
     this._files[fileName] = null;
+    this._changedFiles[fileName] = true;
   }
 
   fileExists(fileName: string): boolean {
diff --git a/packages/@ngtools/webpack/src/lazy_routes.ts b/packages/@ngtools/webpack/src/lazy_routes.ts
@@ -0,0 +1,66 @@
+import {dirname, join} from 'path';
+import * as ts from 'typescript';
+
+import {TypeScriptFileRefactor} from './refactor';
+
+
+function _getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): string {
+  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,
+                               program: ts.Program,
+                               host: ts.CompilerHost): LazyRouteMap {
+  const refactor = new TypeScriptFileRefactor(filePath, host, program);
+
+  return refactor
+    // Find all object literals in the file.
+    .findAstNodes(null, ts.SyntaxKind.ObjectLiteralExpression, true)
+    // Get all their property assignments.
+    .map((node: ts.ObjectLiteralExpression) => {
+      return refactor.findAstNodes(node, ts.SyntaxKind.PropertyAssignment, false);
+    })
+    // Take all `loadChildren` elements.
+    .reduce((acc: ts.PropertyAssignment[], props: ts.PropertyAssignment[]) => {
+      return acc.concat(props.filter(literal => {
+        return _getContentOfKeyLiteral(refactor.sourceFile, 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 resolvedModuleName: ts.ResolvedModuleWithFailedLookupLocations = moduleName[0] == '.'
+        ? { resolvedModule: { resolvedFileName: join(dirname(filePath), moduleName) + '.ts' },
+            failedLookupLocations: [] }
+        : ts.resolveModuleName(moduleName, filePath, program.getCompilerOptions(), 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]) => {
+      acc[routePath] = resolvedModuleName;
+      return acc;
+    }, {});
+}
diff --git a/packages/@ngtools/webpack/src/plugin.ts b/packages/@ngtools/webpack/src/plugin.ts
@@ -11,6 +11,7 @@ import {WebpackCompilerHost} from './compiler_host';
 import {resolveEntryModuleFromMain} from './entry_resolver';
 import {Tapable} from './webpack';
 import {PathsPlugin} from './paths-plugin';
+import {findLazyRoutes, LazyRouteMap} from './lazy_routes';
 
 
 /**
@@ -39,7 +40,7 @@ export class AotPlugin implements Tapable {
   private _rootFilePath: string[];
   private _compilerHost: WebpackCompilerHost;
   private _resourceLoader: WebpackResourceLoader;
-  private _lazyRoutes: { [route: string]: string };
+  private _lazyRoutes: LazyRouteMap = Object.create(null);
   private _tsConfigPath: string;
   private _entryModule: string;
 
@@ -56,6 +57,8 @@ export class AotPlugin implements Tapable {
   private _i18nFormat: string;
   private _locale: string;
 
+  private _firstRun = true;
+
   constructor(options: AotPluginOptions) {
     this._setupOptions(options);
   }
@@ -78,6 +81,7 @@ export class AotPlugin implements Tapable {
   get i18nFile() { return this._i18nFile; }
   get i18nFormat() { return this._i18nFormat; }
   get locale() { return this._locale; }
+  get firstRun() { return this._firstRun; }
 
   private _setupOptions(options: AotPluginOptions) {
     // Fill in the missing options.
@@ -194,6 +198,31 @@ export class AotPlugin implements Tapable {
     }
   }
 
+  private _findLazyRoutesInAst(): LazyRouteMap {
+    const result: LazyRouteMap = Object.create(null);
+    const changedFilePaths = this._compilerHost.getChangedFilePaths();
+    for (const filePath of changedFilePaths) {
+      const fileLazyRoutes = findLazyRoutes(filePath, this._program, this._compilerHost);
+      for (const routeKey of Object.keys(fileLazyRoutes)) {
+        const route = fileLazyRoutes[routeKey];
+        if (routeKey in this._lazyRoutes) {
+          if (route === null) {
+            this._lazyRoutes[routeKey] = null;
+          } else if (this._lazyRoutes[routeKey] !== route) {
+            this._compilation.warnings.push(
+              new Error(`Duplicated path in loadChildren detected during a rebuild. `
+                + `We will take the latest version detected and override it to save rebuild time. `
+                + `You should perform a full build to validate that your routes don't overlap.`)
+            );
+          }
+        } else {
+          result[routeKey] = route;
+        }
+      }
+    }
+    return result;
+  }
+
   // registration hook for webpack plugin
   apply(compiler: any) {
     this._compiler = compiler;
@@ -220,7 +249,15 @@ export class AotPlugin implements Tapable {
           result.dependencies.forEach((d: any) => d.critical = false);
           result.resolveDependencies = (p1: any, p2: any, p3: any, p4: RegExp, cb: any ) => {
             const dependencies = Object.keys(this._lazyRoutes)
-              .map((key) => new ContextElementDependency(this._lazyRoutes[key], key));
+              .map((key) => {
+                const value = this._lazyRoutes[key];
+                if (value !== null) {
+                  return new ContextElementDependency(value, key);
+                } else {
+                  return null;
+                }
+              })
+              .filter(x => !!x);
             cb(null, dependencies);
           };
           return callback(null, result);
@@ -314,18 +351,26 @@ export class AotPlugin implements Tapable {
         this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal);
       })
       .then(() => {
-        // Process the lazy routes
-        this._lazyRoutes = {};
-        const allLazyRoutes = __NGTOOLS_PRIVATE_API_2.listLazyRoutes({
-          program: this._program,
-          host: this._compilerHost,
-          angularCompilerOptions: this._angularCompilerOptions,
-          entryModule: this._entryModule
-        });
-        Object.keys(allLazyRoutes)
+        // We need to run the `listLazyRoutes` the first time because it also navigates libraries
+        // and other things that we might miss using the findLazyRoutesInAst.
+        let discoveredLazyRoutes: LazyRouteMap = this.firstRun ?
+          __NGTOOLS_PRIVATE_API_2.listLazyRoutes({
+            program: this._program,
+            host: this._compilerHost,
+            angularCompilerOptions: this._angularCompilerOptions,
+            entryModule: this._entryModule
+          })
+          : this._findLazyRoutesInAst();
+
+        // Process the lazy routes discovered.
+        Object.keys(discoveredLazyRoutes)
           .forEach(k => {
-            const lazyRoute = allLazyRoutes[k];
+            const lazyRoute = discoveredLazyRoutes[k];
             k = k.split('#')[0];
+            if (lazyRoute === null) {
+              return;
+            }
+
             if (this.skipCodeGeneration) {
               this._lazyRoutes[k] = lazyRoute;
             } else {
@@ -334,7 +379,13 @@ export class AotPlugin implements Tapable {
             }
           });
       })
-      .then(() => cb(), (err: any) => {
+      .then(() => {
+        this._compilerHost.resetChangedFileTracker();
+
+        // Only turn this off for the first successful run.
+        this._firstRun = false;
+        cb();
+      }, (err: any) => {
         compilation.errors.push(err);
         cb();
       });
diff --git a/packages/angular-cli/models/webpack-configs/common.ts b/packages/angular-cli/models/webpack-configs/common.ts
@@ -67,7 +67,7 @@ export function getCommonConfig(wco: WebpackConfigOptions) {
     extraPlugins.push(new webpack.optimize.CommonsChunkPlugin({
       name: 'vendor',
       chunks: ['main'],
-      minChunks: (module: any) => module.userRequest && module.userRequest.startsWith(nodeModules)
+      minChunks: (module: any) => module.resource && module.resource.startsWith(nodeModules)
     }));
   }
 
diff --git a/tests/e2e/tests/build/rebuild.ts b/tests/e2e/tests/build/rebuild.ts
@@ -0,0 +1,73 @@
+import {
+  killAllProcesses,
+  exec,
+  waitForAnyProcessOutputToMatch,
+  silentExecAndWaitForOutputToMatch,
+  ng, execAndWaitForOutputToMatch,
+} from '../../utils/process';
+import {writeFile} from '../../utils/fs';
+import {wait} from '../../utils/utils';
+
+
+export default function() {
+  if (process.platform.startsWith('win')) {
+    return Promise.resolve();
+  }
+
+  let oldNumberOfChunks = 0;
+  const chunkRegExp = /chunk\s+\{/g;
+
+  return execAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
+    // Should trigger a rebuild.
+    .then(() => exec('touch', 'src/main.ts'))
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
+    // Count the bundles.
+    .then((stdout: string) => {
+      oldNumberOfChunks = stdout.split(chunkRegExp).length;
+    })
+    // Add a lazy module.
+    .then(() => ng('generate', 'module', 'lazy', '--routing'))
+    // Just wait for the rebuild, otherwise we might be validating this build.
+    .then(() => wait(1000))
+    .then(() => writeFile('src/app/app.module.ts', `
+      import { BrowserModule } from '@angular/platform-browser';
+      import { NgModule } from '@angular/core';
+      import { FormsModule } from '@angular/forms';
+      import { HttpModule } from '@angular/http';
+      
+      import { AppComponent } from './app.component';
+      import { RouterModule } from '@angular/router';
+      
+      @NgModule({
+        declarations: [
+          AppComponent
+        ],
+        imports: [
+          BrowserModule,
+          FormsModule,
+          HttpModule,
+          RouterModule.forRoot([
+            { path: 'lazy', loadChildren: './lazy/lazy.module#LazyModule' }
+          ])
+        ],
+        providers: [],
+        bootstrap: [AppComponent]
+      })
+      export class AppModule { }
+    `))
+    // Should trigger a rebuild with a new bundle.
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
+    // Count the bundles.
+    .then((stdout: string) => {
+      let newNumberOfChunks = stdout.split(chunkRegExp).length;
+      if (oldNumberOfChunks >= newNumberOfChunks) {
+        throw new Error('Expected webpack to create a new chunk, but did not.');
+      }
+    })
+    .then(() => killAllProcesses(), (err: any) => {
+      killAllProcesses();
+      throw err;
+    });
+}
diff --git a/tests/e2e/tests/packages/webpack/test.ts b/tests/e2e/tests/packages/webpack/test.ts
@@ -16,5 +16,6 @@ export default function(skipCleaning: () => void) {
     .then(() => replaceInFile('app/app.component.ts',
       './app.component.scss', 'app.component.scss'))
     .then(() => exec(normalize('node_modules/.bin/webpack'), '-p'))
+    // test
     .then(() => skipCleaning());
 }
diff --git a/tests/e2e/utils/process.ts b/tests/e2e/utils/process.ts
@@ -83,6 +83,26 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
   });
 }
 
+export function waitForAnyProcessOutputToMatch(match: RegExp, timeout = 30000): Promise<string> {
+  // Race between _all_ processes, and the timeout. First one to resolve/reject wins.
+  return Promise.race(_processes.map(childProcess => new Promise(resolve => {
+    let stdout = '';
+    childProcess.stdout.on('data', (data: Buffer) => {
+      stdout += data.toString();
+      if (data.toString().match(match)) {
+        resolve(stdout);
+      }
+    });
+  })).concat([
+    new Promise((resolve, reject) => {
+      // Wait for 30 seconds and timeout.
+      setTimeout(() => {
+        reject(new Error(`Waiting for ${match} timed out (timeout: ${timeout}msec)...`));
+      }, timeout);
+    })
+  ]));
+}
+
 export function killAllProcesses(signal = 'SIGTERM') {
   _processes.forEach(process => treeKill(process.pid, signal));
   _processes = [];

Original file line number	Diff line number	Diff line change
`@@ -67,7 +67,7 @@ export function getCommonConfig(wco: WebpackConfigOptions) {`
`67`	`67`	`extraPlugins.push(new webpack.optimize.CommonsChunkPlugin({`
`68`	`68`	`name: 'vendor',`
`69`	`69`	`chunks: ['main'],`
`70`		`- minChunks: (module: any) => module.userRequest && module.userRequest.startsWith(nodeModules)`
	`70`	`+ minChunks: (module: any) => module.resource && module.resource.startsWith(nodeModules)`
`71`	`71`	`}));`
`72`	`72`	`}`
`73`	`73`
Original file line number	Diff line number	Diff line change
`@@ -16,5 +16,6 @@ export default function(skipCleaning: () => void) {`
`16`	`16`	`.then(() => replaceInFile('app/app.component.ts',`
`17`	`17`	`'./app.component.scss', 'app.component.scss'))`
`18`	`18`	`.then(() => exec(normalize('node_modules/.bin/webpack'), '-p'))`
	`19`	`+ // test`
`19`	`20`	`.then(() => skipCleaning());`
`20`	`21`	`}`