perf(@ngtools/webpack): reduce rebuild performance by typechecking more (#4258)

We got the TypeScript performance down a lot by not type checking every files on rebuild, but this has an issue where typings can be wrong in files that depends on us. This PR alleviate the problem by type checking all files that depends on the changed file.

Note that even if this PR reduces performance, we're still ~40% faster
than Beta.26.

Author: hansl

package.json

@@ -135,7 +135,7 @@
     "@types/rimraf": "0.0.25-alpha",
     "@types/semver": "^5.3.30",
     "@types/source-map": "^0.5.0",
-    "@types/webpack": "^2.2.1",
+    "@types/webpack": "^2.2.4",
     "chai": "^3.5.0",
     "conventional-changelog": "^1.1.0",
     "dtsgenerator": "^0.7.1",

packages/@ngtools/webpack/src/loader.ts

@@ -2,8 +2,10 @@ import * as path from 'path';
 import * as ts from 'typescript';
 import {AotPlugin} from './plugin';
 import {TypeScriptFileRefactor} from './refactor';
+import {LoaderContext, ModuleReason} from './webpack';
 
 const loaderUtils = require('loader-utils');
+const NormalModule = require('webpack/lib/NormalModule');
 
 
 function _getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): string {
@@ -160,7 +162,7 @@ function _replaceResources(refactor: TypeScriptFileRefactor): void {
 
 
 function _checkDiagnostics(refactor: TypeScriptFileRefactor) {
-  const diagnostics = refactor.getDiagnostics();
+  const diagnostics: ts.Diagnostic[] = refactor.getDiagnostics();
 
   if (diagnostics.length > 0) {
     const message = diagnostics
@@ -175,10 +177,23 @@ function _checkDiagnostics(refactor: TypeScriptFileRefactor) {
 }
 
 
+/**
+ * Recursively calls diagnose on the plugins for all the reverse dependencies.
+ * @private
+ */
+function _diagnoseDeps(reasons: ModuleReason[], plugin: AotPlugin) {
+  reasons
+    .filter(reason => reason && reason.module && reason.module instanceof NormalModule)
+    .forEach(reason => {
+      plugin.diagnose(reason.module.resource);
+      _diagnoseDeps(reason.module.reasons, plugin);
+    });
+}
+
+
 // Super simple TS transpiler loader for testing / isolated usage. does not type check!
-export function ngcLoader(source: string) {
-  this.cacheable();
-  const cb: any = this.async();
+export function ngcLoader(this: LoaderContext & { _compilation: any }) {
+  const cb = this.async();
   const sourceFileName: string = this.resourcePath;
 
   const plugin = this._compilation._ngToolsWebpackPluginInstance as AotPlugin;
@@ -201,7 +216,13 @@ export function ngcLoader(source: string) {
       })
       .then(() => {
         if (plugin.typeCheck) {
-          _checkDiagnostics(refactor);
+          // Check all diagnostics from this and reverse dependencies also.
+          if (!plugin.firstRun) {
+            _diagnoseDeps(this._module.reasons, plugin);
+          }
+          // We do this here because it will throw on error, resulting in rebuilding this file
+          // the next time around if it changes.
+          plugin.diagnose(sourceFileName);
         }
       })
       .then(() => {

packages/@ngtools/webpack/src/plugin.ts

@@ -57,6 +57,7 @@ export class AotPlugin implements Tapable {
   private _i18nFormat: string;
   private _locale: string;
 
+  private _diagnoseFiles: { [path: string]: boolean } = {};
   private _firstRun = true;
 
   constructor(options: AotPluginOptions) {
@@ -227,7 +228,11 @@ export class AotPlugin implements Tapable {
   apply(compiler: any) {
     this._compiler = compiler;
 
-    compiler.plugin('invalid', (fileName: string, timestamp: number) => {
+    compiler.plugin('invalid', (fileName: string) => {
+      // Turn this off as soon as a file becomes invalid and we're about to start a rebuild.
+      this._firstRun = false;
+      this._diagnoseFiles = {};
+
       this._compilerHost.invalidate(fileName);
     });
 
@@ -291,6 +296,37 @@ export class AotPlugin implements Tapable {
     });
   }
 
+  diagnose(fileName: string) {
+    if (this._diagnoseFiles[fileName]) {
+      return;
+    }
+    this._diagnoseFiles[fileName] = true;
+
+    const sourceFile = this._program.getSourceFile(fileName);
+      if (!sourceFile) {
+        return;
+      }
+
+    const diagnostics: ts.Diagnostic[] = []
+      .concat(
+        this._program.getCompilerOptions().declaration
+          ? this._program.getDeclarationDiagnostics(sourceFile) : [],
+        this._program.getSyntacticDiagnostics(sourceFile),
+        this._program.getSemanticDiagnostics(sourceFile)
+      );
+
+    if (diagnostics.length > 0) {
+      const message = diagnostics
+        .map(diagnostic => {
+          const {line, character} = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start);
+          const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, '\n');
+          return `${diagnostic.file.fileName} (${line + 1},${character + 1}): ${message})`;
+        })
+        .join('\n');
+      this._compilation.errors.push(message);
+    }
+  }
+
   private _make(compilation: any, cb: (err?: any, request?: any) => void) {
     this._compilation = compilation;
     if (this._compilation._ngToolsWebpackPluginInstance) {
@@ -382,8 +418,6 @@ export class AotPlugin implements Tapable {
       .then(() => {
         this._compilerHost.resetChangedFileTracker();
 
-        // Only turn this off for the first successful run.
-        this._firstRun = false;
         cb();
       }, (err: any) => {
         compilation.errors.push(err);

packages/@ngtools/webpack/src/webpack.ts

@@ -23,3 +23,29 @@ export interface ResolverPlugin extends Tapable {
   join(relativePath: string, innerRequest: Request): Request;
 }
 
+export interface LoaderCallback {
+  (err: Error | null, source?: string, sourceMap?: string): void;
+}
+
+export interface ModuleReason {
+  dependency: any;
+  module: NormalModule;
+}
+
+export interface NormalModule {
+  buildTimestamp: number;
+  built: boolean;
+  reasons: ModuleReason[];
+  resource: string;
+}
+
+export interface LoaderContext {
+  _module: NormalModule;
+
+  async(): LoaderCallback;
+  cacheable(): void;
+
+  readonly resourcePath: string;
+  readonly query: any;
+}
+

tests/e2e/tests/build/rebuild-deps-type-check.ts

@@ -0,0 +1,63 @@
+import {
+  killAllProcesses,
+  waitForAnyProcessOutputToMatch,
+  silentExecAndWaitForOutputToMatch,
+} from '../../utils/process';
+import {writeFile, prependToFile, appendToFile} from '../../utils/fs';
+import {wait} from '../../utils/utils';
+
+
+export default function() {
+  if (process.platform.startsWith('win')) {
+    return Promise.resolve();
+  }
+
+  return silentExecAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
+    // Create and import files.
+    .then(() => writeFile('src/funky2.ts', `
+      export function funky2(value: string): string {
+        return value + 'hello';
+      }
+    `))
+    .then(() => writeFile('src/funky.ts', `
+      export * from './funky2';
+    `))
+    .then(() => prependToFile('src/main.ts', `
+      import { funky } from './funky';
+    `))
+    .then(() => appendToFile('src/main.ts', `
+      console.log(funky('town'));
+    `))
+    // Should trigger a rebuild, no error expected.
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
+    // Create and import files.
+    .then(() => wait(2000))
+    .then(() => writeFile('src/funky2.ts', `
+      export function funky(value: number): number {
+        return value + 1;
+      }
+    `))
+    // Should trigger a rebuild, this time an error is expected.
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
+    .then(({ stdout }) => {
+      if (!/ERROR in .*\/src\/main\.ts \(/.test(stdout)) {
+        throw new Error('Expected an error but none happened.');
+      }
+    })
+    // Fix the error!
+    .then(() => writeFile('src/funky2.ts', `
+      export function funky(value: string): string {
+        return value + 'hello';
+      }
+    `))
+    .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
+    .then(({ stdout }) => {
+      if (/ERROR in .*\/src\/main\.ts \(/.test(stdout)) {
+        throw new Error('Expected no error but an error was shown.');
+      }
+    })
+    .then(() => killAllProcesses(), (err: any) => {
+      killAllProcesses();
+      throw err;
+    });
+}

tests/e2e/tests/build/rebuild.ts

@@ -3,7 +3,7 @@ import {
   exec,
   waitForAnyProcessOutputToMatch,
   silentExecAndWaitForOutputToMatch,
-  ng, execAndWaitForOutputToMatch,
+  ng,
 } from '../../utils/process';
 import {writeFile} from '../../utils/fs';
 import {wait} from '../../utils/utils';
@@ -17,18 +17,18 @@ export default function() {
   let oldNumberOfChunks = 0;
   const chunkRegExp = /chunk\s+\{/g;
 
-  return execAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
+  return silentExecAndWaitForOutputToMatch('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) => {
+    .then(({ stdout }) => {
       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.
+    // Just wait for the rebuild, otherwise we might be validating the last build.
     .then(() => wait(1000))
     .then(() => writeFile('src/app/app.module.ts', `
       import { BrowserModule } from '@angular/platform-browser';
@@ -60,7 +60,7 @@ export default function() {
     .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
     .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
     // Count the bundles.
-    .then((stdout: string) => {
+    .then(({ stdout }) => {
       let newNumberOfChunks = stdout.split(chunkRegExp).length;
       if (oldNumberOfChunks >= newNumberOfChunks) {
         throw new Error('Expected webpack to create a new chunk, but did not.');

tests/e2e/utils/fs.ts

@@ -103,6 +103,12 @@ export function appendToFile(filePath: string, text: string) {
 }
 
 
+export function prependToFile(filePath: string, text: string) {
+  return readFile(filePath)
+    .then((content: string) => writeFile(filePath, text.concat(content)));
+}
+
+
 export function expectFileToExist(fileName: string) {
   return new Promise((resolve, reject) => {
     fs.exists(fileName, (exist) => {

tests/e2e/utils/process.ts

@@ -11,7 +11,13 @@ interface ExecOptions {
 
 let _processes: child_process.ChildProcess[] = [];
 
-function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<string> {
+type ProcessOutput = {
+  stdout: string;
+  stdout: string;
+};
+
+
+function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<ProcessOutput> {
   let stdout = '';
   let stderr = '';
   const cwd = process.cwd();
@@ -51,6 +57,9 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
   });
   childProcess.stderr.on('data', (data: Buffer) => {
     stderr += data.toString('utf-8');
+    if (options.silent) {
+      return;
+    }
     data.toString('utf-8')
       .split(/[\n\r]+/)
       .filter(line => line !== '')
@@ -76,23 +85,28 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
     if (options.waitForMatch) {
       childProcess.stdout.on('data', (data: Buffer) => {
         if (data.toString().match(options.waitForMatch)) {
-          resolve(stdout);
+          resolve({ stdout, stderr });
         }
       });
     }
   });
 }
 
-export function waitForAnyProcessOutputToMatch(match: RegExp, timeout = 30000): Promise<string> {
+export function waitForAnyProcessOutputToMatch(match: RegExp,
+                                               timeout = 30000): Promise<ProcessOutput> {
   // Race between _all_ processes, and the timeout. First one to resolve/reject wins.
   return Promise.race(_processes.map(childProcess => new Promise(resolve => {
     let stdout = '';
+    let stderr = '';
     childProcess.stdout.on('data', (data: Buffer) => {
       stdout += data.toString();
       if (data.toString().match(match)) {
-        resolve(stdout);
+        resolve({ stdout, stderr });
       }
     });
+    childProcess.stderr.on('data', (data: Buffer) => {
+      stderr += data.toString();
+    });
   })).concat([
     new Promise((resolve, reject) => {
       // Wait for 30 seconds and timeout.