fix(@angular-devkit/build-angular): improve sourcemaps fidelity when code coverage is enabled

With this change we replace `@jsdevtools/coverage-istanbul-loader` webpack loader with [`babel-plugin-istanbul`](https://github.com/istanbuljs/babel-plugin-istanbul) which is an official Babel plugin by the istanbuljs team.

Previously, when code coverage was enabled we had multiple Babel runs on the same file. This is because istanbuljs' `instrumentSync` and `instrument` APIs which are used by the Webpack plugin invokes Babel directly https://github.com/istanbuljs/istanbuljs/blob/66bc39b3c7b301a4b4456101a9996f90b1638dc0/packages/istanbul-lib-instrument/src/instrumenter.js#L98

By using the babel plugin directly, we avoid this which also improves the sourcemaps correctness and test performance.

Closes #22010

Author: alan-agius4

package.json

@@ -92,7 +92,6 @@
     "@bazel/jasmine": "4.4.0",
     "@bazel/typescript": "4.4.0",
     "@discoveryjs/json-ext": "0.5.5",
-    "@jsdevtools/coverage-istanbul-loader": "3.0.5",
     "@types/babel__core": "7.1.16",
     "@types/babel__template": "7.4.1",
     "@types/cacache": "^15.0.0",
@@ -128,6 +127,7 @@
     "ajv-formats": "2.1.1",
     "ansi-colors": "4.1.1",
     "babel-loader": "8.2.3",
+    "babel-plugin-istanbul": "6.1.1",
     "bootstrap": "^4.0.0",
     "browserslist": "^4.9.1",
     "cacache": "15.3.0",

packages/angular_devkit/build_angular/BUILD.bazel

@@ -112,7 +112,6 @@ ts_library(
         "@npm//@babel/runtime",
         "@npm//@babel/template",
         "@npm//@discoveryjs/json-ext",
-        "@npm//@jsdevtools/coverage-istanbul-loader",
         "@npm//@types/babel__core",
         "@npm//@types/babel__template",
         "@npm//@types/browserslist",
@@ -134,6 +133,7 @@ ts_library(
         "@npm//ajv",
         "@npm//ansi-colors",
         "@npm//babel-loader",
+        "@npm//babel-plugin-istanbul",
         "@npm//browserslist",
         "@npm//cacache",
         "@npm//caniuse-lite",

packages/angular_devkit/build_angular/package.json

@@ -20,10 +20,10 @@
     "@babel/runtime": "7.15.4",
     "@babel/template": "7.15.4",
     "@discoveryjs/json-ext": "0.5.5",
-    "@jsdevtools/coverage-istanbul-loader": "3.0.5",
     "@ngtools/webpack": "0.0.0",
     "ansi-colors": "4.1.1",
     "babel-loader": "8.2.3",
+    "babel-plugin-istanbul": "6.1.1",
     "browserslist": "^4.9.1",
     "cacache": "15.3.0",
     "caniuse-lite": "^1.0.30001032",

packages/angular_devkit/build_angular/src/babel/presets/application.ts

@@ -47,6 +47,14 @@ export interface ApplicationPresetOptions {
 
   forceES5?: boolean;
   forceAsyncTransformation?: boolean;
+  instrumentCode?: {
+    includedBasePath: string;
+  };
+  optimize?: {
+    looseEnums: boolean;
+    pureTopLevel: boolean;
+    wrapDecorators: boolean;
+  };
 
   diagnosticReporter?: DiagnosticReporter;
 }
@@ -220,6 +228,31 @@ export default function (api: unknown, options: ApplicationPresetOptions) {
     needRuntimeTransform = true;
   }
 
+  if (options.optimize) {
+    if (options.optimize.pureTopLevel) {
+      plugins.push(require('../plugins/pure-toplevel-functions').default);
+    }
+
+    plugins.push(
+      require('../plugins/elide-angular-metadata').default,
+      [
+        require('../plugins/adjust-typescript-enums').default,
+        { loose: options.optimize.looseEnums },
+      ],
+      [
+        require('../plugins/adjust-static-class-members').default,
+        { wrapDecorators: options.optimize.wrapDecorators },
+      ],
+    );
+  }
+
+  if (options.instrumentCode) {
+    plugins.push([
+      require('babel-plugin-istanbul').default,
+      { inputSourceMap: false, cwd: options.instrumentCode.includedBasePath },
+    ]);
+  }
+
   if (needRuntimeTransform) {
     // Babel equivalent to TypeScript's `importHelpers` option
     plugins.push([

packages/angular_devkit/build_angular/src/babel/webpack-loader.ts

@@ -12,16 +12,16 @@ import { ScriptTarget } from 'typescript';
 import { loadEsmModule } from '../utils/load-esm';
 import { ApplicationPresetOptions, I18nPluginCreators } from './presets/application';
 
-interface AngularCustomOptions extends Pick<ApplicationPresetOptions, 'angularLinker' | 'i18n'> {
-  forceAsyncTransformation: boolean;
-  forceES5: boolean;
-  optimize?: {
-    looseEnums: boolean;
-    pureTopLevel: boolean;
-    wrapDecorators: boolean;
+interface AngularCustomOptions extends Omit<ApplicationPresetOptions, 'instrumentCode'> {
+  instrumentCode?: {
+    /** node_modules and test files are always excluded. */
+    excludedPaths: Set<String>;
+    includedBasePath: string;
   };
 }
 
+export type AngularBabelLoaderOptions = AngularCustomOptions & Record<string, unknown>;
+
 // Extract Sourcemap input type from the remapping function since it is not currently exported
 type SourceMapInput = Exclude<Parameters<typeof remapping>[0], unknown[]>;
 
@@ -62,7 +62,7 @@ async function requiresLinking(path: string, source: string): Promise<boolean> {
   return needsLinking(path, source);
 }
 
-export default custom<AngularCustomOptions>(() => {
+export default custom<ApplicationPresetOptions>(() => {
   const baseOptions = Object.freeze({
     babelrc: false,
     configFile: false,
@@ -73,15 +73,19 @@ export default custom<AngularCustomOptions>(() => {
   });
 
   return {
-    async customOptions({ i18n, scriptTarget, aot, optimize, ...rawOptions }, { source }) {
+    async customOptions(options, { source }) {
+      const { i18n, scriptTarget, aot, optimize, instrumentCode, ...rawOptions } =
+        options as AngularBabelLoaderOptions;
+
       // Must process file if plugins are added
       let shouldProcess = Array.isArray(rawOptions.plugins) && rawOptions.plugins.length > 0;
 
-      const customOptions: AngularCustomOptions = {
+      const customOptions: ApplicationPresetOptions = {
         forceAsyncTransformation: false,
         forceES5: false,
         angularLinker: undefined,
         i18n: undefined,
+        instrumentCode: undefined,
       };
 
       // Analyze file for linking
@@ -117,7 +121,7 @@ export default custom<AngularCustomOptions>(() => {
           customOptions.forceAsyncTransformation =
             !/[\\/][_f]?esm2015[\\/]/.test(this.resourcePath) && source.includes('async');
         }
-        shouldProcess ||= customOptions.forceAsyncTransformation || customOptions.forceES5;
+        shouldProcess ||= customOptions.forceAsyncTransformation || customOptions.forceES5 || false;
       }
 
       // Analyze for i18n inlining
@@ -163,6 +167,20 @@ export default custom<AngularCustomOptions>(() => {
         shouldProcess = true;
       }
 
+      if (
+        instrumentCode &&
+        !instrumentCode.excludedPaths.has(this.resourcePath) &&
+        !/\.(e2e|spec)\.tsx?$|[\\/]node_modules[\\/]/.test(this.resourcePath) &&
+        this.resourcePath.startsWith(instrumentCode.includedBasePath)
+      ) {
+        // `babel-plugin-istanbul` has it's own includes but we do the below so that we avoid running the the loader.
+        customOptions.instrumentCode = {
+          includedBasePath: instrumentCode.includedBasePath,
+        };
+
+        shouldProcess = true;
+      }
+
       // Add provided loader options to default base options
       const loaderOptions: Record<string, unknown> = {
         ...baseOptions,
@@ -184,32 +202,12 @@ export default custom<AngularCustomOptions>(() => {
       return { custom: customOptions, loader: loaderOptions };
     },
     config(configuration, { customOptions }) {
-      const plugins = configuration.options.plugins ?? [];
-      if (customOptions.optimize) {
-        if (customOptions.optimize.pureTopLevel) {
-          plugins.push(require('./plugins/pure-toplevel-functions').default);
-        }
-
-        plugins.push(
-          require('./plugins/elide-angular-metadata').default,
-          [
-            require('./plugins/adjust-typescript-enums').default,
-            { loose: customOptions.optimize.looseEnums },
-          ],
-          [
-            require('./plugins/adjust-static-class-members').default,
-            { wrapDecorators: customOptions.optimize.wrapDecorators },
-          ],
-        );
-      }
-
       return {
         ...configuration.options,
         // Using `false` disables babel from attempting to locate sourcemaps or process any inline maps.
         // The babel types do not include the false option even though it is valid
         // eslint-disable-next-line @typescript-eslint/no-explicit-any
         inputSourceMap: false as any,
-        plugins,
         presets: [
           ...(configuration.options.presets || []),
           [
@@ -240,16 +238,10 @@ export default custom<AngularCustomOptions>(() => {
         // `@ampproject/remapping` source map objects but both are compatible with Webpack.
         // This method for merging is used because it provides more accurate output
         // and is faster while using less memory.
-        result.map = {
-          // Convert the SourceMap back to simple plain object.
-          // This is needed because otherwise code-coverage will fail with `don't know how to turn this value into a node`
-          // Which is throw by Babel when it is invoked again from `istanbul-lib-instrument`.
-          // https://github.com/babel/babel/blob/780aa48d2a34dc55f556843074b6aed45e7eabeb/packages/babel-types/src/converters/valueToNode.ts#L115-L130
-          ...(remapping(
-            [result.map as SourceMapInput, inputSourceMap as SourceMapInput],
-            () => null,
-          ) as typeof result.map),
-        };
+        result.map = remapping(
+          [result.map as SourceMapInput, inputSourceMap as SourceMapInput],
+          () => null,
+        ) as typeof result.map;
       }
 
       return result;

packages/angular_devkit/build_angular/src/builders/karma/tests/options/code-coverage_spec.ts

@@ -6,7 +6,6 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import { last, tap } from 'rxjs/operators';
 import { promisify } from 'util';
 import { execute } from '../../index';
 import { BASE_OPTIONS, KARMA_BUILDER_INFO, describeBuilder } from '../setup';

packages/angular_devkit/build_angular/src/webpack/configs/common.ts

@@ -14,28 +14,37 @@ import { ScriptTarget } from 'typescript';
 import {
   Compiler,
   Configuration,
-  ContextReplacementPlugin,
   ProgressPlugin,
   RuleSetRule,
   WebpackOptionsNormalized,
   debug,
 } from 'webpack';
+import { AngularBabelLoaderOptions } from '../../babel/webpack-loader';
 import { AssetPatternClass } from '../../builders/browser/schema';
 import { BuildBrowserFeatures } from '../../utils';
-import { WebpackConfigOptions } from '../../utils/build-options';
+import { WebpackConfigOptions, WebpackTestOptions } from '../../utils/build-options';
 import { allowMangle, profilingEnabled } from '../../utils/environment-options';
 import { loadEsmModule } from '../../utils/load-esm';
 import { Spinner } from '../../utils/spinner';
 import { addError } from '../../utils/webpack-diagnostics';
 import { DedupeModuleResolvePlugin, ScriptsWebpackPlugin } from '../plugins';
 import { JavaScriptOptimizerPlugin } from '../plugins/javascript-optimizer-plugin';
-import { getOutputHashFormat, getWatchOptions, normalizeExtraEntryPoints } from '../utils/helpers';
+import {
+  getInstrumentationExcludedPaths,
+  getOutputHashFormat,
+  getWatchOptions,
+  normalizeExtraEntryPoints,
+} from '../utils/helpers';
 
 // eslint-disable-next-line max-lines-per-function
-export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Configuration> {
-  const { root, projectRoot, buildOptions, tsConfig, projectName } = wco;
+export async function getCommonConfig(
+  wco: WebpackConfigOptions<WebpackTestOptions>,
+): Promise<Configuration> {
+  const { root, projectRoot, buildOptions, tsConfig, projectName, sourceRoot } = wco;
   const {
     cache,
+    codeCoverage,
+    codeCoverageExclude = [],
     platform = 'browser',
     sourceMap: { styles: stylesSourceMap, scripts: scriptsSourceMap, vendor: vendorSourceMap },
     optimization: { styles: stylesOptimization, scripts: scriptsOptimization },
@@ -380,7 +389,13 @@ export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Config
                 scriptTarget: wco.scriptTarget,
                 aot: buildOptions.aot,
                 optimize: buildOptions.buildOptimizer,
-              },
+                instrumentCode: codeCoverage
+                  ? {
+                      includedBasePath: sourceRoot,
+                      excludedPaths: getInstrumentationExcludedPaths(root, codeCoverageExclude),
+                    }
+                  : undefined,
+              } as AngularBabelLoaderOptions,
             },
           ],
         },

packages/angular_devkit/build_angular/src/webpack/configs/test.ts

@@ -6,44 +6,20 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import * as glob from 'glob';
 import * as path from 'path';
 import { ScriptTarget } from 'typescript';
-import { Configuration, RuleSetRule } from 'webpack';
+import { Configuration } from 'webpack';
 import { WebpackConfigOptions, WebpackTestOptions } from '../../utils/build-options';
 import { getSourceMapDevTool, isPolyfillsEntry } from '../utils/helpers';
 
 export function getTestConfig(wco: WebpackConfigOptions<WebpackTestOptions>): Configuration {
   const {
-    buildOptions: { codeCoverage, codeCoverageExclude, main, sourceMap, webWorkerTsConfig },
+    buildOptions: { main, sourceMap, webWorkerTsConfig },
     root,
-    sourceRoot,
   } = wco;
 
-  const extraRules: RuleSetRule[] = [];
   const extraPlugins: Configuration['plugins'] = [];
 
-  if (codeCoverage) {
-    const exclude: (string | RegExp)[] = [/\.(e2e|spec)\.tsx?$/, /node_modules/];
-
-    if (codeCoverageExclude) {
-      for (const excludeGlob of codeCoverageExclude) {
-        glob
-          .sync(path.join(root, excludeGlob), { nodir: true })
-          .forEach((file) => exclude.push(path.normalize(file)));
-      }
-    }
-
-    extraRules.push({
-      test: /\.[cm]?[tj]sx?$/,
-      loader: require.resolve('@jsdevtools/coverage-istanbul-loader'),
-      options: { esModules: true },
-      enforce: 'post',
-      exclude,
-      include: sourceRoot,
-    });
-  }
-
   if (sourceMap.scripts || sourceMap.styles) {
     extraPlugins.push(getSourceMapDevTool(sourceMap.scripts, sourceMap.styles, false, true));
   }
@@ -60,7 +36,6 @@ export function getTestConfig(wco: WebpackConfigOptions<WebpackTestOptions>): Co
       main: path.resolve(root, main),
     },
     module: {
-      rules: extraRules,
       parser:
         webWorkerTsConfig === undefined
           ? {

packages/angular_devkit/build_angular/src/webpack/utils/helpers.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import glob from 'glob';
 import * as path from 'path';
 import { Configuration, SourceMapDevToolPlugin } from 'webpack';
 import { ExtraEntryPoint, ExtraEntryPointClass } from '../../builders/browser/schema';
@@ -131,3 +132,18 @@ export function assetNameTemplateFactory(hashFormat: HashFormat): (resourcePath:
     return '[path][name].[ext]';
   };
 }
+
+export function getInstrumentationExcludedPaths(
+  sourceRoot: string,
+  excludedPaths: string[],
+): Set<string> {
+  const excluded = new Set<string>();
+
+  for (const excludeGlob of excludedPaths) {
+    glob
+      .sync(path.join(sourceRoot, excludeGlob), { nodir: true })
+      .forEach((p) => excluded.add(path.normalize(p)));
+  }
+
+  return excluded;
+}