Skip to content

Commit 250fae7

Browse files
sdjaynaalan-agius4
authored andcommitted
fix(@angular/build): write the stats file in a stable, deterministic order
Component stylesheet metafiles merged into the main metafile in bundle completion order, so the statsJson stats file varied between builds and, when two stylesheets bundled to the same output file, the last merged won and dropped the other's input attribution. Sort the dynamic path dictionaries (inputs, outputs, and each output's inputs) when serializing the stats file, and union the inputs of colliding outputs so every input is attributed. Both avoid a JSON.stringify replacer and per-collision object spreads, keeping V8 on its native serializer and avoiding the O(n^2) allocations raised in review. (cherry picked from commit b0c415a)
1 parent 9cba0da commit 250fae7

3 files changed

Lines changed: 210 additions & 5 deletions

File tree

‎packages/angular/build/src/builders/application/execute-build.ts‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88

99
import { BuilderContext } from '@angular-devkit/architect';
10+
import type { Metafile } from 'esbuild';
1011
import { createAngularCompilation } from '../../tools/angular/compilation';
1112
import {
1213
AngularCompilationContext,
@@ -43,6 +44,42 @@ import { createComponentStyleBundler, setupBundlerContexts } from './setup-bundl
4344
/** The esbuild error text prefix used to detect top-level await errors. */
4445
const TOP_LEVEL_AWAIT_ERROR_TEXT =
4546
'Top-level await is not available in the configured target environment';
47+
48+
/** Returns a shallow copy of a record with its keys sorted. */
49+
function sortObjectKeys<T extends Record<string, unknown>>(value: T): T {
50+
const sorted: Record<string, unknown> = {};
51+
for (const key of Object.keys(value).sort()) {
52+
sorted[key] = value[key];
53+
}
54+
55+
return sorted as T;
56+
}
57+
58+
/**
59+
* Serializes a metafile with its dynamic path keys sorted so the stats file is identical across
60+
* builds. Only `inputs`, `outputs`, and each output's `inputs` have dynamic keys; the remaining
61+
* fixed-schema objects are left untouched so V8 keeps its native `JSON.stringify` fast path.
62+
*/
63+
function stableStringify(metafile: Metafile): string {
64+
const outputs: Metafile['outputs'] = {};
65+
for (const key of Object.keys(metafile.outputs).sort()) {
66+
const output = metafile.outputs[key];
67+
outputs[key] = {
68+
...output,
69+
inputs: sortObjectKeys(output.inputs),
70+
};
71+
}
72+
73+
return JSON.stringify(
74+
{
75+
inputs: sortObjectKeys(metafile.inputs),
76+
outputs,
77+
},
78+
null,
79+
2,
80+
);
81+
}
82+
4683
// eslint-disable-next-line max-lines-per-function
4784
export async function executeBuild(
4885
options: NormalizedApplicationBuildOptions,
@@ -394,14 +431,14 @@ export async function executeBuild(
394431
if (options.stats) {
395432
executionResult.addOutputFile(
396433
'browser-stats.json',
397-
JSON.stringify(browserMetafile, null, 2),
434+
stableStringify(browserMetafile),
398435
BuildOutputFileType.Root,
399436
);
400437

401438
if (serverEntryPoint) {
402439
executionResult.addOutputFile(
403440
'server-stats.json',
404-
JSON.stringify(serverMetafile, null, 2),
441+
stableStringify(serverMetafile),
405442
BuildOutputFileType.Root,
406443
);
407444
}

‎packages/angular/build/src/builders/application/tests/options/stats-json_spec.ts‎

Lines changed: 156 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,33 @@
77
*/
88

99
import { buildApplication } from '../../index';
10+
import { OutputHashing } from '../../schema';
1011
import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup';
1112

13+
/** Asserts that a record's keys are already in sorted order. */
14+
function expectSortedKeys(record: Record<string, unknown>, context: string): void {
15+
const keys = Object.keys(record);
16+
expect(keys)
17+
.withContext(context)
18+
.toEqual([...keys].sort());
19+
}
20+
21+
/** A standalone component in `src/app/<dir>` that uses the stylesheet `src/app/<dir>/shared.css`. */
22+
function sharedStylesheetComponent(dir: string): string {
23+
return `
24+
import { Component } from '@angular/core';
25+
@Component({
26+
selector: 'app-shared-${dir}',
27+
template: '<p>${dir}</p>',
28+
styleUrl: './shared.css',
29+
})
30+
export class Shared${dir.toUpperCase()}Component {}
31+
`;
32+
}
33+
1234
describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
1335
describe('Option: "statsJson"', () => {
14-
it('generates only browser stats file containing valid metafile data when true', async () => {
36+
it('generates only browser stats file with valid, sorted metafile data when true', async () => {
1537
harness.useTarget('build', {
1638
...BASE_OPTIONS,
1739
statsJson: true,
@@ -27,6 +49,8 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
2749
expect(browserStats.inputs).toBeDefined();
2850
expect(browserStats.outputs).toBeDefined();
2951
expect(Object.keys(browserStats.outputs).length).toBeGreaterThan(0);
52+
expectSortedKeys(browserStats.inputs, 'inputs');
53+
expectSortedKeys(browserStats.outputs, 'outputs');
3054
});
3155

3256
it('does not generate stats files when false', async () => {
@@ -52,6 +76,131 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
5276
harness.expectFile('dist/server-stats.json').toNotExist();
5377
});
5478

79+
describe('shared component stylesheet', () => {
80+
beforeEach(async () => {
81+
// Two components in different directories with byte-identical stylesheets of the same
82+
// file name bundle to the same output file.
83+
await harness.writeFiles({
84+
'src/app/a/shared.component.ts': sharedStylesheetComponent('a'),
85+
'src/app/a/shared.css': 'p { color: red; }',
86+
'src/app/b/shared.component.ts': sharedStylesheetComponent('b'),
87+
'src/app/b/shared.css': 'p { color: red; }',
88+
'src/app/app.component.html':
89+
'<app-shared-a></app-shared-a><app-shared-b></app-shared-b>',
90+
'src/app/app.module.ts': `
91+
import { NgModule } from '@angular/core';
92+
import { BrowserModule } from '@angular/platform-browser';
93+
import { AppComponent } from './app.component';
94+
import { SharedAComponent } from './a/shared.component';
95+
import { SharedBComponent } from './b/shared.component';
96+
@NgModule({
97+
declarations: [AppComponent],
98+
imports: [BrowserModule, SharedAComponent, SharedBComponent],
99+
bootstrap: [AppComponent],
100+
})
101+
export class AppModule {}
102+
`,
103+
});
104+
});
105+
106+
it('attributes a shared component stylesheet output to every input', async () => {
107+
harness.useTarget('build', {
108+
...BASE_OPTIONS,
109+
statsJson: true,
110+
});
111+
112+
const { result } = await harness.executeOnce();
113+
expect(result?.success).toBeTrue();
114+
115+
// Both inputs are attributed to the single shared output, and in sorted order.
116+
const stats = JSON.parse(harness.readFile('dist/browser-stats.json'));
117+
expect(Object.keys(stats.outputs['shared.css'].inputs)).toEqual([
118+
'src/app/a/shared.css',
119+
'src/app/b/shared.css',
120+
]);
121+
});
122+
123+
it('re-attributes a shared component stylesheet output after one stylesheet changes', async () => {
124+
// Hashed output names keep the two stylesheets apart once their content differs, so the
125+
// outputs are found by their inputs. Minified styles carry no source path comment, which
126+
// gives the identical stylesheets the same hashed name.
127+
harness.useTarget('build', {
128+
...BASE_OPTIONS,
129+
statsJson: true,
130+
optimization: { styles: true },
131+
outputHashing: OutputHashing.All,
132+
watch: true,
133+
});
134+
135+
await harness.executeWithCases([
136+
async ({ result }) => {
137+
expect(result?.success).toBeTrue();
138+
const stats = JSON.parse(harness.readFile('dist/browser-stats.json'));
139+
expect(Object.values(stats.outputs)).toContain(
140+
jasmine.objectContaining({
141+
inputs: {
142+
'src/app/a/shared.css': jasmine.anything(),
143+
'src/app/b/shared.css': jasmine.anything(),
144+
},
145+
}),
146+
);
147+
148+
await harness.writeFile('src/app/a/shared.css', 'p { color: blue; }');
149+
},
150+
async ({ result }) => {
151+
expect(result?.success).toBeTrue();
152+
const stats = JSON.parse(harness.readFile('dist/browser-stats.json'));
153+
expect(Object.values(stats.outputs)).toContain(
154+
jasmine.objectContaining({
155+
inputs: { 'src/app/a/shared.css': jasmine.anything() },
156+
}),
157+
);
158+
expect(Object.values(stats.outputs)).toContain(
159+
jasmine.objectContaining({
160+
inputs: { 'src/app/b/shared.css': jasmine.anything() },
161+
}),
162+
);
163+
164+
await harness.writeFile('src/app/a/shared.css', 'p { color: red; }');
165+
},
166+
async ({ result }) => {
167+
expect(result?.success).toBeTrue();
168+
const stats = JSON.parse(harness.readFile('dist/browser-stats.json'));
169+
expect(Object.values(stats.outputs)).toContain(
170+
jasmine.objectContaining({
171+
inputs: {
172+
'src/app/a/shared.css': jasmine.anything(),
173+
'src/app/b/shared.css': jasmine.anything(),
174+
},
175+
}),
176+
);
177+
178+
// The restore re-bundled only `a`, so `b` now merges first. A second change to `a`
179+
// checks that the merge left the cached `b` result untouched.
180+
await harness.writeFile('src/app/a/shared.css', 'p { color: green; }');
181+
},
182+
({ result }) => {
183+
expect(result?.success).toBeTrue();
184+
const stats = JSON.parse(harness.readFile('dist/browser-stats.json'));
185+
// The inner `inputs` objects are matched exactly, not with `objectContaining`: if the
186+
// previous case had mutated the cached `b` result in place, `b` would read
187+
// `{ a, b }` here and this exact match would fail. This is the guard for the
188+
// clone-on-first-insert in the metafile merge, so it must stay an exact match.
189+
expect(Object.values(stats.outputs)).toContain(
190+
jasmine.objectContaining({
191+
inputs: { 'src/app/a/shared.css': jasmine.anything() },
192+
}),
193+
);
194+
expect(Object.values(stats.outputs)).toContain(
195+
jasmine.objectContaining({
196+
inputs: { 'src/app/b/shared.css': jasmine.anything() },
197+
}),
198+
);
199+
},
200+
]);
201+
});
202+
});
203+
55204
describe('server build', () => {
56205
beforeEach(async () => {
57206
await harness.modifyFile('src/tsconfig.app.json', (content) => {
@@ -63,7 +212,7 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
63212
});
64213
});
65214

66-
it('generates separated browser and server stats files for an SSR build', async () => {
215+
it('generates separated browser and server stats files with sorted keys for an SSR build', async () => {
67216
harness.useTarget('build', {
68217
...BASE_OPTIONS,
69218
server: 'src/main.server.ts',
@@ -80,6 +229,11 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
80229
const browserStats = JSON.parse(harness.readFile('dist/browser-stats.json'));
81230
const serverStats = JSON.parse(harness.readFile('dist/server-stats.json'));
82231

232+
expectSortedKeys(browserStats.inputs, 'browser inputs');
233+
expectSortedKeys(browserStats.outputs, 'browser outputs');
234+
expectSortedKeys(serverStats.inputs, 'server inputs');
235+
expectSortedKeys(serverStats.outputs, 'server outputs');
236+
83237
const browserPaths = new Set(Object.keys(browserStats.outputs));
84238
const serverPaths = new Set(Object.keys(serverStats.outputs));
85239

‎packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,21 @@ export function createCompilerPlugin(
639639
// mitigating significant performance overhead for large apps.
640640
// See: https://bugs.chromium.org/p/v8/issues/detail?id=11536
641641
Object.assign(result.metafile.inputs, metafile.inputs);
642-
Object.assign(result.metafile.outputs, metafile.outputs);
642+
for (const [outputPath, output] of Object.entries(metafile.outputs)) {
643+
// Two stylesheets can bundle to the same output file. Clone the inputs on first
644+
// insert so colliding inputs can be unioned in place with Object.assign, keeping
645+
// every input attributed without per-collision spread allocations and leaving the
646+
// cached `additionalResults` metafile untouched.
647+
const existing = result.metafile.outputs[outputPath];
648+
if (existing) {
649+
Object.assign(existing.inputs, output.inputs);
650+
} else {
651+
result.metafile.outputs[outputPath] = {
652+
...output,
653+
inputs: { ...output.inputs },
654+
};
655+
}
656+
}
643657
}
644658
}
645659

0 commit comments

Comments
 (0)