Skip to content

Commit f679585

Browse files
clydinclydin
committed
fix(@angular-devkit/build-angular): propagate localize errors to full build result
When using the `application` or `browser-esbuild` builders, localization errors were previously not being propagated to the final build result. In the event that the `i18nMissingTranslation` option was changed to error from the default of warning, the final build result would previously indicate a success if there were any missing translations present within the build. This has now been corrected and the build will now fully fail including a non-zero exit code in this case.
1 parent 43f22f4 commit f679585

File tree

4 files changed

+264
-15
lines changed

4 files changed

+264
-15
lines changed

‎packages/angular_devkit/build_angular/src/builders/application/i18n.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,13 @@ export async function inlineI18n(
5252
try {
5353
for (const locale of options.i18nOptions.inlineLocales) {
5454
// A locale specific set of files is returned from the inliner.
55-
const localeOutputFiles = await inliner.inlineForLocale(
55+
const localeInlineResult = await inliner.inlineForLocale(
5656
locale,
5757
options.i18nOptions.locales[locale].translation,
5858
);
59+
const localeOutputFiles = localeInlineResult.outputFiles;
60+
inlineResult.errors.push(...localeInlineResult.errors);
61+
inlineResult.warnings.push(...localeInlineResult.warnings);
5962

6063
const baseHref =
6164
getLocaleBaseHref(options.baseHref, options.i18nOptions, locale) ?? options.baseHref;

‎packages/angular_devkit/build_angular/src/builders/application/tests/options/i18n-missing-translation_spec.ts

+224
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import { buildApplication } from '../../index';
10+
import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup';
11+
12+
describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
13+
describe('Option: "i18nMissingTranslation"', () => {
14+
beforeEach(() => {
15+
harness.useProject('test', {
16+
root: '.',
17+
sourceRoot: 'src',
18+
cli: {
19+
cache: {
20+
enabled: false,
21+
},
22+
},
23+
i18n: {
24+
locales: {
25+
'fr': 'src/locales/messages.fr.xlf',
26+
},
27+
},
28+
});
29+
});
30+
31+
it('should warn when i18nMissingTranslation is undefined (default)', async () => {
32+
harness.useTarget('build', {
33+
...BASE_OPTIONS,
34+
localize: true,
35+
i18nMissingTranslation: undefined,
36+
});
37+
38+
await harness.writeFile(
39+
'src/app/app.component.html',
40+
`
41+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
42+
`,
43+
);
44+
45+
await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);
46+
47+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
48+
49+
expect(result?.success).toBeTrue();
50+
expect(logs).toContain(
51+
jasmine.objectContaining({
52+
level: 'warn',
53+
message: jasmine.stringMatching('No translation found for'),
54+
}),
55+
);
56+
});
57+
58+
it('should warn when i18nMissingTranslation is set to warning', async () => {
59+
harness.useTarget('build', {
60+
...BASE_OPTIONS,
61+
localize: true,
62+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
63+
i18nMissingTranslation: 'warning' as any,
64+
});
65+
66+
await harness.writeFile(
67+
'src/app/app.component.html',
68+
`
69+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
70+
`,
71+
);
72+
73+
await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);
74+
75+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
76+
77+
expect(result?.success).toBeTrue();
78+
expect(logs).toContain(
79+
jasmine.objectContaining({
80+
level: 'warn',
81+
message: jasmine.stringMatching('No translation found for'),
82+
}),
83+
);
84+
});
85+
86+
it('should error when i18nMissingTranslation is set to error', async () => {
87+
harness.useTarget('build', {
88+
...BASE_OPTIONS,
89+
localize: true,
90+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
91+
i18nMissingTranslation: 'error' as any,
92+
});
93+
94+
await harness.writeFile(
95+
'src/app/app.component.html',
96+
`
97+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
98+
`,
99+
);
100+
101+
await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);
102+
103+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
104+
105+
expect(result?.success).toBeFalse();
106+
expect(logs).toContain(
107+
jasmine.objectContaining({
108+
level: 'error',
109+
message: jasmine.stringMatching('No translation found for'),
110+
}),
111+
);
112+
});
113+
114+
it('should not error or warn when i18nMissingTranslation is set to ignore', async () => {
115+
harness.useTarget('build', {
116+
...BASE_OPTIONS,
117+
localize: true,
118+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
119+
i18nMissingTranslation: 'ignore' as any,
120+
});
121+
122+
await harness.writeFile(
123+
'src/app/app.component.html',
124+
`
125+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
126+
`,
127+
);
128+
129+
await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);
130+
131+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
132+
133+
expect(result?.success).toBeTrue();
134+
expect(logs).not.toContain(
135+
jasmine.objectContaining({
136+
message: jasmine.stringMatching('No translation found for'),
137+
}),
138+
);
139+
});
140+
141+
it('should not error or warn when i18nMissingTranslation is set to error and all found', async () => {
142+
harness.useTarget('build', {
143+
...BASE_OPTIONS,
144+
localize: true,
145+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
146+
i18nMissingTranslation: 'error' as any,
147+
});
148+
149+
await harness.writeFile(
150+
'src/app/app.component.html',
151+
`
152+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
153+
`,
154+
);
155+
156+
await harness.writeFile('src/locales/messages.fr.xlf', GOOD_TRANSLATION_FILE_CONTENT);
157+
158+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
159+
160+
expect(result?.success).toBeTrue();
161+
expect(logs).not.toContain(
162+
jasmine.objectContaining({
163+
message: jasmine.stringMatching('No translation found for'),
164+
}),
165+
);
166+
});
167+
168+
it('should not error or warn when i18nMissingTranslation is set to warning and all found', async () => {
169+
harness.useTarget('build', {
170+
...BASE_OPTIONS,
171+
localize: true,
172+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
173+
i18nMissingTranslation: 'warning' as any,
174+
});
175+
176+
await harness.writeFile(
177+
'src/app/app.component.html',
178+
`
179+
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
180+
`,
181+
);
182+
183+
await harness.writeFile('src/locales/messages.fr.xlf', GOOD_TRANSLATION_FILE_CONTENT);
184+
185+
const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });
186+
187+
expect(result?.success).toBeTrue();
188+
expect(logs).not.toContain(
189+
jasmine.objectContaining({
190+
message: jasmine.stringMatching('No translation found for'),
191+
}),
192+
);
193+
});
194+
});
195+
});
196+
197+
const GOOD_TRANSLATION_FILE_CONTENT = `
198+
<?xml version="1.0" encoding="UTF-8" ?>
199+
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
200+
<file target-language="fr" datatype="plaintext" original="ng2.template">
201+
<body>
202+
<trans-unit id="4286451273117902052" datatype="html">
203+
<target>Bonjour <x id="INTERPOLATION" equiv-text="{{ title }}"/>! </target>
204+
<context-group purpose="location">
205+
<context context-type="targetfile">src/app/app.component.html</context>
206+
<context context-type="linenumber">2,3</context>
207+
</context-group>
208+
<note priority="1" from="description">An introduction header for this sample</note>
209+
</trans-unit>
210+
</body>
211+
</file>
212+
</xliff>
213+
`;
214+
215+
const MISSING_TRANSLATION_FILE_CONTENT = `
216+
<?xml version="1.0" encoding="UTF-8" ?>
217+
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
218+
<file target-language="fr" datatype="plaintext" original="ng2.template">
219+
<body>
220+
221+
</body>
222+
</file>
223+
</xliff>
224+
`;

‎packages/angular_devkit/build_angular/src/tools/esbuild/i18n-inliner-worker.ts

+6-8
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,12 @@ export default async function inlineLocale(request: InlineRequest) {
6060
request,
6161
);
6262

63-
// TODO: Return diagnostics
64-
// TODO: Consider buffer transfer instead of string copying
65-
const response = [{ file: request.filename, contents: result.code }];
66-
if (result.map) {
67-
response.push({ file: request.filename + '.map', contents: result.map });
68-
}
69-
70-
return response;
63+
return {
64+
file: request.filename,
65+
code: result.code,
66+
map: result.map,
67+
messages: result.diagnostics.messages,
68+
};
7169
}
7270

7371
/**

‎packages/angular_devkit/build_angular/src/tools/esbuild/i18n-inliner.ts

+30-6
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 assert from 'node:assert';
910
import Piscina from 'piscina';
1011
import { BuildOutputFile, BuildOutputFileType } from './bundler-context';
1112
import { createOutputFileFromText } from './utils';
@@ -110,7 +111,7 @@ export class I18nInliner {
110111
async inlineForLocale(
111112
locale: string,
112113
translation: Record<string, unknown> | undefined,
113-
): Promise<BuildOutputFile[]> {
114+
): Promise<{ outputFiles: BuildOutputFile[]; errors: string[]; warnings: string[] }> {
114115
// Request inlining for each file that contains localize calls
115116
const requests = [];
116117
for (const filename of this.#localizeFiles.keys()) {
@@ -130,13 +131,36 @@ export class I18nInliner {
130131
const rawResults = await Promise.all(requests);
131132

132133
// Convert raw results to output file objects and include all unmodified files
133-
return [
134-
...rawResults.flat().map(({ file, contents }) =>
135-
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
136-
createOutputFileFromText(file, contents, this.#fileToType.get(file)!),
137-
),
134+
const errors: string[] = [];
135+
const warnings: string[] = [];
136+
const outputFiles = [
137+
...rawResults.flatMap(({ file, code, map, messages }) => {
138+
const type = this.#fileToType.get(file);
139+
assert(type !== undefined, 'localized file should always have a type' + file);
140+
141+
const resultFiles = [createOutputFileFromText(file, code, type)];
142+
if (map) {
143+
resultFiles.push(createOutputFileFromText(file + '.map', map, type));
144+
}
145+
146+
for (const message of messages) {
147+
if (message.type === 'error') {
148+
errors.push(message.message);
149+
} else {
150+
warnings.push(message.message);
151+
}
152+
}
153+
154+
return resultFiles;
155+
}),
138156
...this.#unmodifiedFiles.map((file) => file.clone()),
139157
];
158+
159+
return {
160+
outputFiles,
161+
errors,
162+
warnings,
163+
};
140164
}
141165

142166
/**

0 commit comments

Comments
 (0)