Skip to content

Commit b14e0a5

Browse files
alan-agius4filipesilva
alan-agius4
authored and
filipesilva
committed
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 (cherry picked from commit cf77b73)
1 parent 0ee6a45 commit b14e0a5

File tree

10 files changed

+155
-101
lines changed

10 files changed

+155
-101
lines changed

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@
9292
"@bazel/jasmine": "4.4.0",
9393
"@bazel/typescript": "4.4.0",
9494
"@discoveryjs/json-ext": "0.5.5",
95-
"@jsdevtools/coverage-istanbul-loader": "3.0.5",
9695
"@types/babel__core": "7.1.16",
9796
"@types/babel__template": "7.4.1",
9897
"@types/cacache": "^15.0.0",
@@ -128,6 +127,7 @@
128127
"ajv-formats": "2.1.1",
129128
"ansi-colors": "4.1.1",
130129
"babel-loader": "8.2.3",
130+
"babel-plugin-istanbul": "6.1.1",
131131
"bootstrap": "^4.0.0",
132132
"browserslist": "^4.9.1",
133133
"cacache": "15.3.0",

‎packages/angular_devkit/build_angular/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ ts_library(
112112
"@npm//@babel/runtime",
113113
"@npm//@babel/template",
114114
"@npm//@discoveryjs/json-ext",
115-
"@npm//@jsdevtools/coverage-istanbul-loader",
116115
"@npm//@types/babel__core",
117116
"@npm//@types/babel__template",
118117
"@npm//@types/browserslist",
@@ -134,6 +133,7 @@ ts_library(
134133
"@npm//ajv",
135134
"@npm//ansi-colors",
136135
"@npm//babel-loader",
136+
"@npm//babel-plugin-istanbul",
137137
"@npm//browserslist",
138138
"@npm//cacache",
139139
"@npm//caniuse-lite",

‎packages/angular_devkit/build_angular/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@
2020
"@babel/runtime": "7.15.4",
2121
"@babel/template": "7.15.4",
2222
"@discoveryjs/json-ext": "0.5.5",
23-
"@jsdevtools/coverage-istanbul-loader": "3.0.5",
2423
"@ngtools/webpack": "0.0.0",
2524
"ansi-colors": "4.1.1",
2625
"babel-loader": "8.2.3",
26+
"babel-plugin-istanbul": "6.1.1",
2727
"browserslist": "^4.9.1",
2828
"cacache": "15.3.0",
2929
"caniuse-lite": "^1.0.30001032",

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,14 @@ export interface ApplicationPresetOptions {
4747

4848
forceES5?: boolean;
4949
forceAsyncTransformation?: boolean;
50+
instrumentCode?: {
51+
includedBasePath: string;
52+
};
53+
optimize?: {
54+
looseEnums: boolean;
55+
pureTopLevel: boolean;
56+
wrapDecorators: boolean;
57+
};
5058

5159
diagnosticReporter?: DiagnosticReporter;
5260
}
@@ -220,6 +228,31 @@ export default function (api: unknown, options: ApplicationPresetOptions) {
220228
needRuntimeTransform = true;
221229
}
222230

231+
if (options.optimize) {
232+
if (options.optimize.pureTopLevel) {
233+
plugins.push(require('../plugins/pure-toplevel-functions').default);
234+
}
235+
236+
plugins.push(
237+
require('../plugins/elide-angular-metadata').default,
238+
[
239+
require('../plugins/adjust-typescript-enums').default,
240+
{ loose: options.optimize.looseEnums },
241+
],
242+
[
243+
require('../plugins/adjust-static-class-members').default,
244+
{ wrapDecorators: options.optimize.wrapDecorators },
245+
],
246+
);
247+
}
248+
249+
if (options.instrumentCode) {
250+
plugins.push([
251+
require('babel-plugin-istanbul').default,
252+
{ inputSourceMap: false, cwd: options.instrumentCode.includedBasePath },
253+
]);
254+
}
255+
223256
if (needRuntimeTransform) {
224257
// Babel equivalent to TypeScript's `importHelpers` option
225258
plugins.push([

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

Lines changed: 33 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,16 @@ import { ScriptTarget } from 'typescript';
1212
import { loadEsmModule } from '../utils/load-esm';
1313
import { ApplicationPresetOptions, I18nPluginCreators } from './presets/application';
1414

15-
interface AngularCustomOptions extends Pick<ApplicationPresetOptions, 'angularLinker' | 'i18n'> {
16-
forceAsyncTransformation: boolean;
17-
forceES5: boolean;
18-
optimize?: {
19-
looseEnums: boolean;
20-
pureTopLevel: boolean;
21-
wrapDecorators: boolean;
15+
interface AngularCustomOptions extends Omit<ApplicationPresetOptions, 'instrumentCode'> {
16+
instrumentCode?: {
17+
/** node_modules and test files are always excluded. */
18+
excludedPaths: Set<String>;
19+
includedBasePath: string;
2220
};
2321
}
2422

23+
export type AngularBabelLoaderOptions = AngularCustomOptions & Record<string, unknown>;
24+
2525
// Extract Sourcemap input type from the remapping function since it is not currently exported
2626
type SourceMapInput = Exclude<Parameters<typeof remapping>[0], unknown[]>;
2727

@@ -62,7 +62,7 @@ async function requiresLinking(path: string, source: string): Promise<boolean> {
6262
return needsLinking(path, source);
6363
}
6464

65-
export default custom<AngularCustomOptions>(() => {
65+
export default custom<ApplicationPresetOptions>(() => {
6666
const baseOptions = Object.freeze({
6767
babelrc: false,
6868
configFile: false,
@@ -73,15 +73,19 @@ export default custom<AngularCustomOptions>(() => {
7373
});
7474

7575
return {
76-
async customOptions({ i18n, scriptTarget, aot, optimize, ...rawOptions }, { source }) {
76+
async customOptions(options, { source }) {
77+
const { i18n, scriptTarget, aot, optimize, instrumentCode, ...rawOptions } =
78+
options as AngularBabelLoaderOptions;
79+
7780
// Must process file if plugins are added
7881
let shouldProcess = Array.isArray(rawOptions.plugins) && rawOptions.plugins.length > 0;
7982

80-
const customOptions: AngularCustomOptions = {
83+
const customOptions: ApplicationPresetOptions = {
8184
forceAsyncTransformation: false,
8285
forceES5: false,
8386
angularLinker: undefined,
8487
i18n: undefined,
88+
instrumentCode: undefined,
8589
};
8690

8791
// Analyze file for linking
@@ -117,7 +121,7 @@ export default custom<AngularCustomOptions>(() => {
117121
customOptions.forceAsyncTransformation =
118122
!/[\\/][_f]?esm2015[\\/]/.test(this.resourcePath) && source.includes('async');
119123
}
120-
shouldProcess ||= customOptions.forceAsyncTransformation || customOptions.forceES5;
124+
shouldProcess ||= customOptions.forceAsyncTransformation || customOptions.forceES5 || false;
121125
}
122126

123127
// Analyze for i18n inlining
@@ -163,6 +167,20 @@ export default custom<AngularCustomOptions>(() => {
163167
shouldProcess = true;
164168
}
165169

170+
if (
171+
instrumentCode &&
172+
!instrumentCode.excludedPaths.has(this.resourcePath) &&
173+
!/\.(e2e|spec)\.tsx?$|[\\/]node_modules[\\/]/.test(this.resourcePath) &&
174+
this.resourcePath.startsWith(instrumentCode.includedBasePath)
175+
) {
176+
// `babel-plugin-istanbul` has it's own includes but we do the below so that we avoid running the the loader.
177+
customOptions.instrumentCode = {
178+
includedBasePath: instrumentCode.includedBasePath,
179+
};
180+
181+
shouldProcess = true;
182+
}
183+
166184
// Add provided loader options to default base options
167185
const loaderOptions: Record<string, unknown> = {
168186
...baseOptions,
@@ -184,32 +202,12 @@ export default custom<AngularCustomOptions>(() => {
184202
return { custom: customOptions, loader: loaderOptions };
185203
},
186204
config(configuration, { customOptions }) {
187-
const plugins = configuration.options.plugins ?? [];
188-
if (customOptions.optimize) {
189-
if (customOptions.optimize.pureTopLevel) {
190-
plugins.push(require('./plugins/pure-toplevel-functions').default);
191-
}
192-
193-
plugins.push(
194-
require('./plugins/elide-angular-metadata').default,
195-
[
196-
require('./plugins/adjust-typescript-enums').default,
197-
{ loose: customOptions.optimize.looseEnums },
198-
],
199-
[
200-
require('./plugins/adjust-static-class-members').default,
201-
{ wrapDecorators: customOptions.optimize.wrapDecorators },
202-
],
203-
);
204-
}
205-
206205
return {
207206
...configuration.options,
208207
// Using `false` disables babel from attempting to locate sourcemaps or process any inline maps.
209208
// The babel types do not include the false option even though it is valid
210209
// eslint-disable-next-line @typescript-eslint/no-explicit-any
211210
inputSourceMap: false as any,
212-
plugins,
213211
presets: [
214212
...(configuration.options.presets || []),
215213
[
@@ -240,16 +238,10 @@ export default custom<AngularCustomOptions>(() => {
240238
// `@ampproject/remapping` source map objects but both are compatible with Webpack.
241239
// This method for merging is used because it provides more accurate output
242240
// and is faster while using less memory.
243-
result.map = {
244-
// Convert the SourceMap back to simple plain object.
245-
// This is needed because otherwise code-coverage will fail with `don't know how to turn this value into a node`
246-
// Which is throw by Babel when it is invoked again from `istanbul-lib-instrument`.
247-
// https://github.com/babel/babel/blob/780aa48d2a34dc55f556843074b6aed45e7eabeb/packages/babel-types/src/converters/valueToNode.ts#L115-L130
248-
...(remapping(
249-
[result.map as SourceMapInput, inputSourceMap as SourceMapInput],
250-
() => null,
251-
) as typeof result.map),
252-
};
241+
result.map = remapping(
242+
[result.map as SourceMapInput, inputSourceMap as SourceMapInput],
243+
() => null,
244+
) as typeof result.map;
253245
}
254246

255247
return result;

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import { last, tap } from 'rxjs/operators';
109
import { promisify } from 'util';
1110
import { execute } from '../../index';
1211
import { BASE_OPTIONS, KARMA_BUILDER_INFO, describeBuilder } from '../setup';

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

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,28 +14,37 @@ import { ScriptTarget } from 'typescript';
1414
import {
1515
Compiler,
1616
Configuration,
17-
ContextReplacementPlugin,
1817
ProgressPlugin,
1918
RuleSetRule,
2019
WebpackOptionsNormalized,
2120
debug,
2221
} from 'webpack';
22+
import { AngularBabelLoaderOptions } from '../../babel/webpack-loader';
2323
import { AssetPatternClass } from '../../builders/browser/schema';
2424
import { BuildBrowserFeatures } from '../../utils';
25-
import { WebpackConfigOptions } from '../../utils/build-options';
25+
import { WebpackConfigOptions, WebpackTestOptions } from '../../utils/build-options';
2626
import { allowMangle, profilingEnabled } from '../../utils/environment-options';
2727
import { loadEsmModule } from '../../utils/load-esm';
2828
import { Spinner } from '../../utils/spinner';
2929
import { addError } from '../../utils/webpack-diagnostics';
3030
import { DedupeModuleResolvePlugin, ScriptsWebpackPlugin } from '../plugins';
3131
import { JavaScriptOptimizerPlugin } from '../plugins/javascript-optimizer-plugin';
32-
import { getOutputHashFormat, getWatchOptions, normalizeExtraEntryPoints } from '../utils/helpers';
32+
import {
33+
getInstrumentationExcludedPaths,
34+
getOutputHashFormat,
35+
getWatchOptions,
36+
normalizeExtraEntryPoints,
37+
} from '../utils/helpers';
3338

3439
// eslint-disable-next-line max-lines-per-function
35-
export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Configuration> {
36-
const { root, projectRoot, buildOptions, tsConfig, projectName } = wco;
40+
export async function getCommonConfig(
41+
wco: WebpackConfigOptions<WebpackTestOptions>,
42+
): Promise<Configuration> {
43+
const { root, projectRoot, buildOptions, tsConfig, projectName, sourceRoot } = wco;
3744
const {
3845
cache,
46+
codeCoverage,
47+
codeCoverageExclude = [],
3948
platform = 'browser',
4049
sourceMap: { styles: stylesSourceMap, scripts: scriptsSourceMap, vendor: vendorSourceMap },
4150
optimization: { styles: stylesOptimization, scripts: scriptsOptimization },
@@ -380,7 +389,13 @@ export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Config
380389
scriptTarget: wco.scriptTarget,
381390
aot: buildOptions.aot,
382391
optimize: buildOptions.buildOptimizer,
383-
},
392+
instrumentCode: codeCoverage
393+
? {
394+
includedBasePath: sourceRoot,
395+
excludedPaths: getInstrumentationExcludedPaths(root, codeCoverageExclude),
396+
}
397+
: undefined,
398+
} as AngularBabelLoaderOptions,
384399
},
385400
],
386401
},

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

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import * as glob from 'glob';
109
import * as path from 'path';
1110
import { ScriptTarget } from 'typescript';
1211
import * as webpack from 'webpack';
@@ -17,34 +16,11 @@ export function getTestConfig(
1716
wco: WebpackConfigOptions<WebpackTestOptions>,
1817
): webpack.Configuration {
1918
const {
20-
buildOptions: { codeCoverage, codeCoverageExclude, main, sourceMap, webWorkerTsConfig },
19+
buildOptions: { main, sourceMap, webWorkerTsConfig },
2120
root,
22-
sourceRoot,
2321
} = wco;
2422

25-
const extraRules: webpack.RuleSetRule[] = [];
26-
const extraPlugins: { apply(compiler: webpack.Compiler): void }[] = [];
27-
28-
if (codeCoverage) {
29-
const exclude: (string | RegExp)[] = [/\.(e2e|spec)\.tsx?$/, /node_modules/];
30-
31-
if (codeCoverageExclude) {
32-
for (const excludeGlob of codeCoverageExclude) {
33-
glob
34-
.sync(path.join(root, excludeGlob), { nodir: true })
35-
.forEach((file) => exclude.push(path.normalize(file)));
36-
}
37-
}
38-
39-
extraRules.push({
40-
test: /\.[cm]?[tj]sx?$/,
41-
loader: require.resolve('@jsdevtools/coverage-istanbul-loader'),
42-
options: { esModules: true },
43-
enforce: 'post',
44-
exclude,
45-
include: sourceRoot,
46-
});
47-
}
23+
const extraPlugins: webpack.Configuration['plugins'] = [];
4824

4925
if (sourceMap.scripts || sourceMap.styles) {
5026
extraPlugins.push(getSourceMapDevTool(sourceMap.scripts, sourceMap.styles, false, true));
@@ -62,7 +38,6 @@ export function getTestConfig(
6238
main: path.resolve(root, main),
6339
},
6440
module: {
65-
rules: extraRules,
6641
parser:
6742
webWorkerTsConfig === undefined
6843
? {

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import glob from 'glob';
910
import * as path from 'path';
1011
import { Configuration, SourceMapDevToolPlugin } from 'webpack';
1112
import { ExtraEntryPoint, ExtraEntryPointClass } from '../../builders/browser/schema';
@@ -131,3 +132,18 @@ export function assetNameTemplateFactory(hashFormat: HashFormat): (resourcePath:
131132
return '[path][name].[ext]';
132133
};
133134
}
135+
136+
export function getInstrumentationExcludedPaths(
137+
sourceRoot: string,
138+
excludedPaths: string[],
139+
): Set<string> {
140+
const excluded = new Set<string>();
141+
142+
for (const excludeGlob of excludedPaths) {
143+
glob
144+
.sync(path.join(sourceRoot, excludeGlob), { nodir: true })
145+
.forEach((p) => excluded.add(path.normalize(p)));
146+
}
147+
148+
return excluded;
149+
}

0 commit comments

Comments
 (0)