From 357aa4a097d1f023af96ee45a99d2d986b9a7093 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 13 Sep 2019 12:46:05 +0100 Subject: [PATCH] fix(ivy): i18n - use `MessageId` for matching translations (#32594) As discussed in https://hackmd.io/33M5Wb-JT7-0fneA0JuHPA `SourceMessage` strings are not sufficient for matching translations. This commit updates `@angular/localize` to use `MessageId`s for translation matching instead. Also the run-time translation will now log a warning to the console if a translation is missing. BREAKING CHANGE: Translations (loaded via the `loadTranslations()` function) must now use `MessageId` for the translation key rather than the previous `SourceMessage` string. PR Close #32594 --- packages/compiler/src/i18n/index.ts | 2 +- packages/localize/BUILD.bazel | 1 + packages/localize/src/translate.ts | 22 ++- packages/localize/src/utils/constants.ts | 23 +++ packages/localize/src/utils/messages.ts | 140 +++++++++++++++--- packages/localize/src/utils/translations.ts | 5 +- packages/localize/test/translate_spec.ts | 26 +++- packages/localize/test/utils/messages_spec.ts | 109 +++++++++++++- .../localize/test/utils/translations_spec.ts | 5 +- tools/public_api_guard/localize/localize.d.ts | 2 +- 10 files changed, 288 insertions(+), 47 deletions(-) diff --git a/packages/compiler/src/i18n/index.ts b/packages/compiler/src/i18n/index.ts index f47e971fd8..5417eecf5d 100644 --- a/packages/compiler/src/i18n/index.ts +++ b/packages/compiler/src/i18n/index.ts @@ -5,7 +5,7 @@ * 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 */ - +export {computeMsgId} from './digest'; export {Extractor, ExtractorHost} from './extractor'; export {I18NHtmlParser} from './i18n_html_parser'; export {MessageBundle} from './message_bundle'; diff --git a/packages/localize/BUILD.bazel b/packages/localize/BUILD.bazel index b05ebec99e..7c98d93264 100644 --- a/packages/localize/BUILD.bazel +++ b/packages/localize/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( ), module_name = "@angular/localize", deps = [ + "//packages/compiler", "//packages/localize/src/localize", "@npm//@types/node", ], diff --git a/packages/localize/src/translate.ts b/packages/localize/src/translate.ts index 12c438ae80..78f52acc8f 100644 --- a/packages/localize/src/translate.ts +++ b/packages/localize/src/translate.ts @@ -15,15 +15,24 @@ import {ParsedTranslation, parseTranslation, translate as _translate} from './ut * Note that because the TRANSLATIONS are attached to a global object, they will be shared between * all applications that are running in a single page of the browser. */ -declare const $localize: LocalizeFn&{TRANSLATIONS: Record}; +declare const $localize: LocalizeFn&{TRANSLATIONS: Record}; /** * Load translations for `$localize`. * - * The given `translations` are processed and added to a lookup based on their translation key. - * A new translation will overwrite a previous translation if it has the same key. + * The given `translations` are processed and added to a lookup based on their `MessageId`. + * A new translation will overwrite a previous translation if it has the same `MessageId`. + * + * * If a message is generated by the Angular compiler from an `i18n` marker in a template, the + * `MessageId` is passed through to the `$localize` call as a custom `MessageId`. The `MessageId` + * will match what is extracted into translation files. + * + * * If the translation is from a call to `$localize` in application code, and no custom `MessageId` + * is provided, then the `MessageId` can be generated by passing the tagged string message-parts + * to the `parseMessage()` function (not currently public API). * * @publicApi + * */ export function loadTranslations(translations: Record) { // Ensure the translate function exists @@ -54,5 +63,10 @@ export function clearTranslations() { */ export function translate(messageParts: TemplateStringsArray, substitutions: readonly any[]): [TemplateStringsArray, readonly any[]] { - return _translate($localize.TRANSLATIONS, messageParts, substitutions); + try { + return _translate($localize.TRANSLATIONS, messageParts, substitutions); + } catch (e) { + console.warn(e.message); + return [messageParts, substitutions]; + } } diff --git a/packages/localize/src/utils/constants.ts b/packages/localize/src/utils/constants.ts index 1bb88a14b4..af571a5afd 100644 --- a/packages/localize/src/utils/constants.ts +++ b/packages/localize/src/utils/constants.ts @@ -19,3 +19,26 @@ * ``` */ export const BLOCK_MARKER = ':'; + +/** + * The marker used to separate a message's "meaning" from its "description" in a metadata block. + * + * For example: + * + * ```ts + * $localize `:correct|Indicates that the user got the answer correct: Right!`; + * $localize `:movement|Button label for moving to the right: Right!`; + * ``` + */ +export const MEANING_SEPARATOR = '|'; + +/** + * The marker used to separate a message's custom "id" from its "description" in a metadata block. + * + * For example: + * + * ```ts + * $localize `:A welcome message on the home page@@myApp-homepage-welcome: Welcome!`; + * ``` + */ +export const ID_SEPARATOR = '@@'; diff --git a/packages/localize/src/utils/messages.ts b/packages/localize/src/utils/messages.ts index c0182f87e1..b908a3ed59 100644 --- a/packages/localize/src/utils/messages.ts +++ b/packages/localize/src/utils/messages.ts @@ -5,7 +5,14 @@ * 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 {BLOCK_MARKER} from './constants'; +import {computeMsgId} from '@angular/compiler'; +import {BLOCK_MARKER, ID_SEPARATOR, MEANING_SEPARATOR} from './constants'; + +/** + * Re-export this helper function so that users of `@angular/localize` don't need to actively import + * from `@angular/compiler`. + */ +export {computeMsgId} from '@angular/compiler'; /** * A string containing a translation source message. @@ -58,6 +65,10 @@ export interface ParsedMessage { * A mapping of placeholder names to substitution values. */ substitutions: Record; + /** + * A human readable rendering of the message + */ + messageString: string; } /** @@ -66,29 +77,112 @@ export interface ParsedMessage { * See `ParsedMessage` for an example. */ export function parseMessage( - messageParts: TemplateStringsArray, expressions: readonly any[]): ParsedMessage { - const replacements: {[placeholderName: string]: any} = {}; - let messageId = messageParts[0]; + messageParts: TemplateStringsArray, expressions?: readonly any[]): ParsedMessage { + const substitutions: {[placeholderName: string]: any} = {}; + const metadata = parseMetadata(messageParts[0], messageParts.raw[0]); + let messageString = metadata.text; for (let i = 1; i < messageParts.length; i++) { - const messagePart = messageParts[i]; - const expression = expressions[i - 1]; - // There is a problem with synthesizing template literals in TS. - // It is not possible to provide raw values for the `messageParts` and TS is not able to compute - // them since this requires access to the string in its original (non-existent) source code. - // Therefore we fall back on the non-raw version if the raw string is empty. - // This should be OK because synthesized nodes only come from the template compiler and they - // will always contain placeholder name information. - // So there will be no escaped placeholder marker character (`:`) directly after a substitution. - if ((messageParts.raw[i] || messagePart).charAt(0) === BLOCK_MARKER) { - const endOfPlaceholderName = messagePart.indexOf(BLOCK_MARKER, 1); - const placeholderName = messagePart.substring(1, endOfPlaceholderName); - messageId += `{$${placeholderName}}${messagePart.substring(endOfPlaceholderName + 1)}`; - replacements[placeholderName] = expression; - } else { - const placeholderName = `ph_${i}`; - messageId += `{$${placeholderName}}${messagePart}`; - replacements[placeholderName] = expression; + const {text: messagePart, block: placeholderName = `ph_${i}`} = + splitBlock(messageParts[i], messageParts.raw[i]); + messageString += `{$${placeholderName}}${messagePart}`; + if (expressions !== undefined) { + substitutions[placeholderName] = expressions[i - 1]; } } - return {messageId: messageId, substitutions: replacements}; + return { + messageId: metadata.id || computeMsgId(messageString, metadata.meaning || ''), + substitutions, + messageString, + }; +} + +export interface MessageMetadata { + text: string; + meaning: string|undefined; + description: string|undefined; + id: string|undefined; +} + +/** + * Parse the given message part (`cooked` + `raw`) to extract the message metadata from the text. + * + * If the message part has a metadata block this function will extract the `meaning`, + * `description` and `id` (if provided) from the block. These metadata properties are serialized in + * the string delimited by `|` and `@@` respectively. + * + * For example: + * + * ```ts + * `:meaning|description@@id` + * `:meaning|@@id` + * `:meaning|description` + * `description@@id` + * `meaning|` + * `description` + * `@@id` + * ``` + * + * @param cooked The cooked version of the message part to parse. + * @param raw The raw version of the message part to parse. + * @returns A object containing any metadata that was parsed from the message part. + */ +export function parseMetadata(cooked: string, raw: string): MessageMetadata { + const {text, block} = splitBlock(cooked, raw); + if (block === undefined) { + return {text, meaning: undefined, description: undefined, id: undefined}; + } else { + const [meaningAndDesc, id] = block.split(ID_SEPARATOR, 2); + let [meaning, description]: (string | undefined)[] = meaningAndDesc.split(MEANING_SEPARATOR, 2); + if (description === undefined) { + description = meaning; + meaning = undefined; + } + if (description === '') { + description = undefined; + } + return {text, meaning, description, id}; + } +} + +/** + * Split a message part (`cooked` + `raw`) into an optional delimited "block" off the front and the + * rest of the text of the message part. + * + * Blocks appear at the start of message parts. They are delimited by a colon `:` character at the + * start and end of the block. + * + * If the block is in the first message part then it will be metadata about the whole message: + * meaning, description, id. Otherwise it will be metadata about the immediately preceding + * substitution: placeholder name. + * + * Since blocks are optional, it is possible that the content of a message block actually starts + * with a block marker. In this case the marker must be escaped `\:`. + * + * @param cooked The cooked version of the message part to parse. + * @param raw The raw version of the message part to parse. + * @returns An object containing the `text` of the message part and the text of the `block`, if it + * exists. + */ +export function splitBlock(cooked: string, raw: string): {text: string, block?: string} { + // Synthesizing AST nodes that represent template literals using the TypeScript API is problematic + // because it doesn't allow for the raw value of messageParts to be programmatically set. + // The result is that synthesized AST nodes have empty `raw` values. + + // Normally we rely upon checking the `raw` value to check whether the `BLOCK_MARKER` was escaped + // in the original source. If the `raw` value is missing then we cannot do this. + // In such a case we fall back on the `cooked` version and assume that the `BLOCK_MARKER` was not + // escaped. + + // This should be OK because synthesized nodes only come from the Angular template compiler, which + // always provides full id and placeholder name information so it will never escape `BLOCK_MARKER` + // characters. + if ((raw || cooked).charAt(0) !== BLOCK_MARKER) { + return {text: cooked}; + } else { + const endOfBlock = cooked.indexOf(BLOCK_MARKER, 1); + return { + block: cooked.substring(1, endOfBlock), + text: cooked.substring(endOfBlock + 1), + }; + } } diff --git a/packages/localize/src/utils/translations.ts b/packages/localize/src/utils/translations.ts index fb054dabc4..c27310114f 100644 --- a/packages/localize/src/utils/translations.ts +++ b/packages/localize/src/utils/translations.ts @@ -33,7 +33,7 @@ export type ParsedTranslations = Record; * `substitutions`. * The translation may reorder (or remove) substitutions as appropriate. * - * If no translation matches then the original `messageParts` and `substitutions` are returned + * If no translation matches then an error is thrown. */ export function translate( translations: Record, messageParts: TemplateStringsArray, @@ -46,7 +46,8 @@ export function translate( translation.placeholderNames.map(placeholder => message.substitutions[placeholder]) ]; } else { - return [messageParts, substitutions]; + throw new Error( + `No translation found for "${message.messageId}" ("${message.messageString}").`); } } diff --git a/packages/localize/test/translate_spec.ts b/packages/localize/test/translate_spec.ts index d54f2793cc..93b6b051b9 100644 --- a/packages/localize/test/translate_spec.ts +++ b/packages/localize/test/translate_spec.ts @@ -5,19 +5,22 @@ * 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 */ +// Ensure that `$localize` is loaded to the global scope. import '@angular/localize/init'; + import {clearTranslations, loadTranslations} from '../src/translate'; +import {MessageId, TargetMessage, computeMsgId} from '../src/utils/messages'; describe('$localize tag with translations', () => { describe('identities', () => { beforeEach(() => { - loadTranslations({ + loadTranslations(computeIds({ 'abc': 'abc', 'abc{$ph_1}': 'abc{$ph_1}', 'abc{$ph_1}def': 'abc{$ph_1}def', 'abc{$ph_1}def{$ph_2}': 'abc{$ph_1}def{$ph_2}', 'Hello, {$ph_1}!': 'Hello, {$ph_1}!', - }); + })); }); afterEach(() => { clearTranslations(); }); @@ -33,13 +36,13 @@ describe('$localize tag with translations', () => { describe('to upper-case messageParts', () => { beforeEach(() => { - loadTranslations({ + loadTranslations(computeIds({ 'abc': 'ABC', 'abc{$ph_1}': 'ABC{$ph_1}', 'abc{$ph_1}def': 'ABC{$ph_1}DEF', 'abc{$ph_1}def{$ph_2}': 'ABC{$ph_1}DEF{$ph_2}', 'Hello, {$ph_1}!': 'HELLO, {$ph_1}!', - }); + })); }); afterEach(() => { clearTranslations(); }); @@ -55,9 +58,9 @@ describe('$localize tag with translations', () => { describe('to reverse expressions', () => { beforeEach(() => { - loadTranslations({ + loadTranslations(computeIds({ 'abc{$ph_1}def{$ph_2} - Hello, {$ph_3}!': 'abc{$ph_3}def{$ph_2} - Hello, {$ph_1}!', - }); + })); }); afterEach(() => { clearTranslations(); }); @@ -70,9 +73,9 @@ describe('$localize tag with translations', () => { describe('to remove expressions', () => { beforeEach(() => { - loadTranslations({ + loadTranslations(computeIds({ 'abc{$ph_1}def{$ph_2} - Hello, {$ph_3}!': 'abc{$ph_1} - Hello, {$ph_3}!', - }); + })); }); afterEach(() => { clearTranslations(); }); @@ -83,3 +86,10 @@ describe('$localize tag with translations', () => { }); }); }); + +function computeIds(translations: Record): + Record { + const processed: Record = {}; + Object.keys(translations).forEach(key => processed[computeMsgId(key, '')] = translations[key]); + return processed; +} diff --git a/packages/localize/test/utils/messages_spec.ts b/packages/localize/test/utils/messages_spec.ts index 632c11b1ad..17b6444834 100644 --- a/packages/localize/test/utils/messages_spec.ts +++ b/packages/localize/test/utils/messages_spec.ts @@ -5,32 +5,58 @@ * 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 {parseMessage} from '../../src/utils/messages'; +import {parseMessage, parseMetadata, splitBlock} from '../../src/utils/messages'; import {makeTemplateObject} from '../../src/utils/translations'; describe('messages utils', () => { describe('parseMessage', () => { - it('should compute the translation key', () => { + it('should use the message-id parsed from the metadata if available', () => { + const message = parseMessage( + makeTemplateObject( + [':@@custom-message-id:a', ':one:b', ':two:c'], + [':@@custom-message-id:a', ':one:b', ':two:c']), + [1, 2]); + expect(message.messageId).toEqual('custom-message-id'); + }); + + it('should compute the translation key if no metadata', () => { const message = parseMessage( makeTemplateObject(['a', ':one:b', ':two:c'], ['a', ':one:b', ':two:c']), [1, 2]); - expect(message.messageId).toEqual('a{$one}b{$two}c'); + expect(message.messageId).toEqual('8865273085679272414'); + }); + + it('should compute the translation key if no id in the metadata', () => { + const message = parseMessage( + makeTemplateObject( + [':description:a', ':one:b', ':two:c'], [':description:a', ':one:b', ':two:c']), + [1, 2]); + expect(message.messageId).toEqual('8865273085679272414'); + }); + + it('should compute a different id if the meaning changes', () => { + const message1 = parseMessage(makeTemplateObject(['abc'], ['abc']), []); + const message2 = parseMessage(makeTemplateObject([':meaning1|:abc'], [':meaning1|:abc']), []); + const message3 = parseMessage(makeTemplateObject([':meaning2|:abc'], [':meaning2|:abc']), []); + expect(message1.messageId).not.toEqual(message2.messageId); + expect(message2.messageId).not.toEqual(message3.messageId); + expect(message3.messageId).not.toEqual(message1.messageId); }); it('should compute the translation key, inferring placeholder names if not given', () => { const message = parseMessage(makeTemplateObject(['a', 'b', 'c'], ['a', 'b', 'c']), [1, 2]); - expect(message.messageId).toEqual('a{$ph_1}b{$ph_2}c'); + expect(message.messageId).toEqual('3269094494609300850'); }); it('should compute the translation key, ignoring escaped placeholder names', () => { const message = parseMessage( makeTemplateObject(['a', ':one:b', ':two:c'], ['a', '\\:one:b', '\\:two:c']), [1, 2]); - expect(message.messageId).toEqual('a{$ph_1}:one:b{$ph_2}:two:c'); + expect(message.messageId).toEqual('529036009514785949'); }); it('should compute the translation key, handling empty raw values', () => { const message = parseMessage(makeTemplateObject(['a', ':one:b', ':two:c'], ['', '', '']), [1, 2]); - expect(message.messageId).toEqual('a{$one}b{$two}c'); + expect(message.messageId).toEqual('8865273085679272414'); }); it('should build a map of named placeholders to expressions', () => { @@ -43,5 +69,76 @@ describe('messages utils', () => { const message = parseMessage(makeTemplateObject(['a', 'b', 'c'], ['a', 'b', 'c']), [1, 2]); expect(message.substitutions).toEqual({ph_1: 1, ph_2: 2}); }); + + }); + + describe('splitBlock()', () => { + it('should return just the text if there is no block', + () => { expect(splitBlock('abc def', 'abc def')).toEqual({text: 'abc def'}); }); + + it('should return just the text and block if there is one', () => { + expect(splitBlock(':block info:abc def', ':block info:abc def')) + .toEqual({text: 'abc def', block: 'block info'}); + }); + + it('should handle an empty block if there is one', () => { + expect(splitBlock('::abc def', '::abc def')).toEqual({text: 'abc def', block: ''}); + }); + + it('should handle escaped block markers', () => { + expect(splitBlock(':part of the message:abc def', '\\:part of the message:abc def')).toEqual({ + text: ':part of the message:abc def' + }); + }); + + it('should handle the empty raw part', () => { + expect(splitBlock(':block info:abc def', '')).toEqual({text: 'abc def', block: 'block info'}); + }); + }); + + describe('parseMetadata()', () => { + it('should return just the text if there is no block', () => { + expect(parseMetadata('abc def', 'abc def')) + .toEqual({text: 'abc def', meaning: undefined, description: undefined, id: undefined}); + }); + + it('should extract the metadata if provided', () => { + expect(parseMetadata(':description:abc def', ':description:abc def')) + .toEqual( + {text: 'abc def', description: 'description', meaning: undefined, id: undefined}); + expect(parseMetadata(':meaning|:abc def', ':meaning|:abc def')) + .toEqual({text: 'abc def', description: undefined, meaning: 'meaning', id: undefined}); + expect(parseMetadata(':@@message-id:abc def', ':@@message-id:abc def')) + .toEqual({text: 'abc def', description: undefined, meaning: undefined, id: 'message-id'}); + expect(parseMetadata(':meaning|description:abc def', ':meaning|description:abc def')) + .toEqual( + {text: 'abc def', description: 'description', meaning: 'meaning', id: undefined}); + expect(parseMetadata(':description@@message-id:abc def', ':description@@message-id:abc def')) + .toEqual( + {text: 'abc def', description: 'description', meaning: undefined, id: 'message-id'}); + expect(parseMetadata(':meaning|@@message-id:abc def', ':meaning|@@message-id:abc def')) + .toEqual({text: 'abc def', description: undefined, meaning: 'meaning', id: 'message-id'}); + }); + + it('should handle an empty block if there is one', () => { + expect(parseMetadata('::abc def', '::abc def')) + .toEqual({text: 'abc def', meaning: undefined, description: undefined, id: undefined}); + }); + + it('should handle escaped block markers', () => { + expect(parseMetadata(':part of the message:abc def', '\\:part of the message:abc def')) + .toEqual({ + text: ':part of the message:abc def', + meaning: undefined, + description: undefined, + id: undefined + }); + }); + + it('should handle the empty raw part', () => { + expect(parseMetadata(':description:abc def', '')) + .toEqual( + {text: 'abc def', meaning: undefined, description: 'description', id: undefined}); + }); }); }); diff --git a/packages/localize/test/utils/translations_spec.ts b/packages/localize/test/utils/translations_spec.ts index da8ab2c3c4..a1e0966b3d 100644 --- a/packages/localize/test/utils/translations_spec.ts +++ b/packages/localize/test/utils/translations_spec.ts @@ -5,7 +5,7 @@ * 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 {TargetMessage} from '@angular/localize/src/utils/messages'; +import {TargetMessage, computeMsgId} from '../../src/utils/messages'; import {ParsedTranslation, makeTemplateObject, parseTranslation, translate} from '../../src/utils/translations'; describe('utils', () => { @@ -151,7 +151,8 @@ describe('utils', () => { Record { const parsedTranslations: Record = {}; Object.keys(translations).forEach(key => { - parsedTranslations[key] = parseTranslation(translations[key]); + + parsedTranslations[computeMsgId(key, '')] = parseTranslation(translations[key]); }); return parsedTranslations; } diff --git a/tools/public_api_guard/localize/localize.d.ts b/tools/public_api_guard/localize/localize.d.ts index c3d5f94d6e..3776ccded2 100644 --- a/tools/public_api_guard/localize/localize.d.ts +++ b/tools/public_api_guard/localize/localize.d.ts @@ -1,3 +1,3 @@ export declare function clearTranslations(): void; -export declare function loadTranslations(translations: Record): void; +export declare function loadTranslations(translations: Record): void;