Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): propagate localize errors to full…
Browse files Browse the repository at this point in the history
… 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.

(cherry picked from commit f679585)
  • Loading branch information
clydin committed Nov 28, 2023
1 parent 22880d9 commit 27e7c2e
Show file tree
Hide file tree
Showing 4 changed files with 264 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@ export async function inlineI18n(
try {
for (const locale of options.i18nOptions.inlineLocales) {
// A locale specific set of files is returned from the inliner.
const localeOutputFiles = await inliner.inlineForLocale(
const localeInlineResult = await inliner.inlineForLocale(
locale,
options.i18nOptions.locales[locale].translation,
);
const localeOutputFiles = localeInlineResult.outputFiles;
inlineResult.errors.push(...localeInlineResult.errors);
inlineResult.warnings.push(...localeInlineResult.warnings);

const baseHref =
getLocaleBaseHref(options.baseHref, options.i18nOptions, locale) ?? options.baseHref;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import { buildApplication } from '../../index';
import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup';

describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
describe('Option: "i18nMissingTranslation"', () => {
beforeEach(() => {
harness.useProject('test', {
root: '.',
sourceRoot: 'src',
cli: {
cache: {
enabled: false,
},
},
i18n: {
locales: {
'fr': 'src/locales/messages.fr.xlf',
},
},
});
});

it('should warn when i18nMissingTranslation is undefined (default)', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
i18nMissingTranslation: undefined,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeTrue();
expect(logs).toContain(
jasmine.objectContaining({
level: 'warn',
message: jasmine.stringMatching('No translation found for'),
}),
);
});

it('should warn when i18nMissingTranslation is set to warning', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
i18nMissingTranslation: 'warning' as any,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeTrue();
expect(logs).toContain(
jasmine.objectContaining({
level: 'warn',
message: jasmine.stringMatching('No translation found for'),
}),
);
});

it('should error when i18nMissingTranslation is set to error', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
i18nMissingTranslation: 'error' as any,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeFalse();
expect(logs).toContain(
jasmine.objectContaining({
level: 'error',
message: jasmine.stringMatching('No translation found for'),
}),
);
});

it('should not error or warn when i18nMissingTranslation is set to ignore', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
i18nMissingTranslation: 'ignore' as any,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', MISSING_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeTrue();
expect(logs).not.toContain(
jasmine.objectContaining({
message: jasmine.stringMatching('No translation found for'),
}),
);
});

it('should not error or warn when i18nMissingTranslation is set to error and all found', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
i18nMissingTranslation: 'error' as any,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', GOOD_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeTrue();
expect(logs).not.toContain(
jasmine.objectContaining({
message: jasmine.stringMatching('No translation found for'),
}),
);
});

it('should not error or warn when i18nMissingTranslation is set to warning and all found', async () => {
harness.useTarget('build', {
...BASE_OPTIONS,
localize: true,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
i18nMissingTranslation: 'warning' as any,
});

await harness.writeFile(
'src/app/app.component.html',
`
<p id="hello" i18n="An introduction header for this sample">Hello {{ title }}! </p>
`,
);

await harness.writeFile('src/locales/messages.fr.xlf', GOOD_TRANSLATION_FILE_CONTENT);

const { result, logs } = await harness.executeOnce({ outputLogsOnFailure: false });

expect(result?.success).toBeTrue();
expect(logs).not.toContain(
jasmine.objectContaining({
message: jasmine.stringMatching('No translation found for'),
}),
);
});
});
});

const GOOD_TRANSLATION_FILE_CONTENT = `
<?xml version="1.0" encoding="UTF-8" ?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
<file target-language="fr" datatype="plaintext" original="ng2.template">
<body>
<trans-unit id="4286451273117902052" datatype="html">
<target>Bonjour <x id="INTERPOLATION" equiv-text="{{ title }}"/>! </target>
<context-group purpose="location">
<context context-type="targetfile">src/app/app.component.html</context>
<context context-type="linenumber">2,3</context>
</context-group>
<note priority="1" from="description">An introduction header for this sample</note>
</trans-unit>
</body>
</file>
</xliff>
`;

const MISSING_TRANSLATION_FILE_CONTENT = `
<?xml version="1.0" encoding="UTF-8" ?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
<file target-language="fr" datatype="plaintext" original="ng2.template">
<body>
</body>
</file>
</xliff>
`;
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,12 @@ export default async function inlineLocale(request: InlineRequest) {
request,
);

// TODO: Return diagnostics
// TODO: Consider buffer transfer instead of string copying
const response = [{ file: request.filename, contents: result.code }];
if (result.map) {
response.push({ file: request.filename + '.map', contents: result.map });
}

return response;
return {
file: request.filename,
code: result.code,
map: result.map,
messages: result.diagnostics.messages,
};
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import assert from 'node:assert';
import Piscina from 'piscina';
import { BuildOutputFile, BuildOutputFileType } from './bundler-context';
import { createOutputFileFromText } from './utils';
Expand Down Expand Up @@ -110,7 +111,7 @@ export class I18nInliner {
async inlineForLocale(
locale: string,
translation: Record<string, unknown> | undefined,
): Promise<BuildOutputFile[]> {
): Promise<{ outputFiles: BuildOutputFile[]; errors: string[]; warnings: string[] }> {
// Request inlining for each file that contains localize calls
const requests = [];
for (const filename of this.#localizeFiles.keys()) {
Expand All @@ -130,13 +131,36 @@ export class I18nInliner {
const rawResults = await Promise.all(requests);

// Convert raw results to output file objects and include all unmodified files
return [
...rawResults.flat().map(({ file, contents }) =>
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
createOutputFileFromText(file, contents, this.#fileToType.get(file)!),
),
const errors: string[] = [];
const warnings: string[] = [];
const outputFiles = [
...rawResults.flatMap(({ file, code, map, messages }) => {
const type = this.#fileToType.get(file);
assert(type !== undefined, 'localized file should always have a type' + file);

const resultFiles = [createOutputFileFromText(file, code, type)];
if (map) {
resultFiles.push(createOutputFileFromText(file + '.map', map, type));
}

for (const message of messages) {
if (message.type === 'error') {
errors.push(message.message);
} else {
warnings.push(message.message);
}
}

return resultFiles;
}),
...this.#unmodifiedFiles.map((file) => file.clone()),
];

return {
outputFiles,
errors,
warnings,
};
}

/**
Expand Down

0 comments on commit 27e7c2e

Please sign in to comment.