From 293ead9407033e5560e4268017af93771a603826 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Thu, 10 Oct 2024 17:06:50 -0700 Subject: [PATCH] fix(compiler): ignore placeholder-only i18n messages Message which only contain a single placeholder cannot be translated, there is no static text to be translated. Therefore these messages can be skipped and shouldn't be extracted at all. Ideally, Angular would throw an error if a message is only a placeholder, since it should not contain an `i18n` attribute at all. However this would be a breaking change and require a migration which isn't in scope right now. We can explore converting this to a hard error sometime in the future. --- .../compiler/src/i18n/extractor_merger.ts | 55 ++++++++++++++++++- .../test/i18n/extractor_merger_spec.ts | 8 +++ .../test/i18n/whitespace_sensitivity_spec.ts | 20 +++---- 3 files changed, 72 insertions(+), 11 deletions(-) diff --git a/packages/compiler/src/i18n/extractor_merger.ts b/packages/compiler/src/i18n/extractor_merger.ts index e7844d033a8a..7002873e0db4 100644 --- a/packages/compiler/src/i18n/extractor_merger.ts +++ b/packages/compiler/src/i18n/extractor_merger.ts @@ -9,6 +9,7 @@ import * as html from '../ml_parser/ast'; import {DEFAULT_CONTAINER_BLOCKS, InterpolationConfig} from '../ml_parser/defaults'; import {ParseTreeResult} from '../ml_parser/parser'; +import {TokenType} from '../ml_parser/tokens'; import * as i18n from './i18n_ast'; import {createI18nMessageFactory, I18nMessageFactory} from './i18n_parser'; @@ -380,7 +381,9 @@ class _Visitor implements html.Visitor { private _addMessage(ast: html.Node[], msgMeta?: string): i18n.Message | null { if ( ast.length == 0 || - (ast.length == 1 && ast[0] instanceof html.Attribute && !(ast[0]).value) + this._isEmptyAttributeValue(ast) || + this._isPlaceholderOnlyAttributeValue(ast) || + this._isPlaceholderOnlyMessage(ast) ) { // Do not create empty messages return null; @@ -392,6 +395,48 @@ class _Visitor implements html.Visitor { return message; } + // Check for cases like `
`. + private _isEmptyAttributeValue(ast: html.Node[]): boolean { + if (!isAttrNode(ast)) return false; + const node = ast[0]; + + return node.value.trim() === ''; + } + + // Check for cases like `
`. + private _isPlaceholderOnlyAttributeValue(ast: html.Node[]): boolean { + if (!isAttrNode(ast)) return false; + const tokens = ast[0].valueTokens ?? []; + + const interpolations = tokens.filter( + (token) => token.type === TokenType.ATTR_VALUE_INTERPOLATION, + ); + const plainText = tokens + .filter((token) => token.type === TokenType.ATTR_VALUE_TEXT) + // `AttributeValueTextToken` always has exactly one part per its type. + .map((token) => token.parts[0].trim()) + .join(''); + + // Check if there is a single interpolation and all text around it is empty. + return interpolations.length === 1 && plainText === ''; + } + + // Check for cases like `
{{ name }}
`. + private _isPlaceholderOnlyMessage(ast: html.Node[]): boolean { + if (!isTextNode(ast)) return false; + const tokens = ast[0].tokens; + + const interpolations = tokens.filter((token) => token.type === TokenType.INTERPOLATION); + const plainText = tokens + .filter((token) => token.type === TokenType.TEXT) + // `TextToken` always has exactly one part per its type. + .map((token) => token.parts[0].trim()) + .join(''); + + // Check if there is a single interpolation and all text around it is empty. + return interpolations.length === 1 && plainText === ''; + } + // Translates the given message given the `TranslationBundle` // This is used for translating elements / blocks - see `_translateAttributes` for attributes // no-op when called in extraction mode (returns []) @@ -593,3 +638,11 @@ function _parseMessageMeta(i18n?: string): {meaning: string; description: string return {meaning, description, id: id.trim()}; } + +function isTextNode(ast: html.Node[]): ast is [html.Text] { + return ast.length === 1 && ast[0] instanceof html.Text; +} + +function isAttrNode(ast: html.Node[]): ast is [html.Attribute] { + return ast.length === 1 && ast[0] instanceof html.Attribute; +} diff --git a/packages/compiler/test/i18n/extractor_merger_spec.ts b/packages/compiler/test/i18n/extractor_merger_spec.ts index af6a66d4f18d..aea01bd54893 100644 --- a/packages/compiler/test/i18n/extractor_merger_spec.ts +++ b/packages/compiler/test/i18n/extractor_merger_spec.ts @@ -100,6 +100,10 @@ describe('Extractor', () => { expect(extract('
')).toEqual([]); }); + it('should not create a message for placeholder-only elements', () => { + expect(extract('
{{ foo }}
')).toEqual([]); + }); + it('should ignore implicit elements in translatable elements', () => { expect(extract('

', ['p'])).toEqual([ [[''], 'm', 'd', ''], @@ -406,6 +410,10 @@ describe('Extractor', () => { it('should not create a message for empty attributes', () => { expect(extract('
')).toEqual([]); }); + + it('should not create a message for placeholder-only attributes', () => { + expect(extract('
')).toEqual([]); + }); }); describe('implicit elements', () => { diff --git a/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts b/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts index a515826ccb23..db479b2b768f 100644 --- a/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts +++ b/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts @@ -18,7 +18,7 @@ describe('i18nPreserveWhitespaceForLegacyExtraction', () => { const initial = extractMessages( `
Hello, World!
-
{{ abc }}
+
Hello {{ abc }}
Start {{ abc }} End
{{ first }} middle {{ end }}
First Second
@@ -41,7 +41,7 @@ Test case is disabled by omitting the i18n attribute. Hello, World!
- {{ abc }} + Hello {{ abc }}
Start {{ abc }} End @@ -111,7 +111,7 @@ Test case is disabled by omitting the i18n attribute. Hello, World!
- {{ abc }} + Hello {{ abc }}
Start {{ abc }} End @@ -171,7 +171,7 @@ Test case is disabled by omitting the i18n attribute. Hello, World!
- {{ abc }} + Hello {{ abc }}
Start {{ abc }} End @@ -237,7 +237,7 @@ Test case is disabled by omitting the i18n attribute. exceeds line length.
- {{ veryLongExpressionWhichMaybeExceedsLineLength | async }} + Hello {{ veryLongExpressionWhichMaybeExceedsLineLength | async }}
This is a long {{ abc }} which maybe @@ -280,7 +280,7 @@ Test case is disabled by omitting the i18n attribute. maybe exceeds line length.
- {{ + Hello {{ veryLongExpressionWhichMaybeExceedsLineLength | async }} @@ -346,7 +346,7 @@ Test case is disabled by omitting the i18n attribute. const initial = extractMessages( `
Hello, World!
-
{{ abc }}
+
Hello {{ abc }}
Start {{ abc }} End
{{ first }} middle {{ end }}
Foo
@@ -357,7 +357,7 @@ Test case is disabled by omitting the i18n attribute. i18nPreserveWhitespaceForLegacyExtraction does not support trimming ICU case text. Test case is disabled by omitting the i18n attribute. -
{ +
Hello { apples, plural, =1 { One apple. } =other { Many apples. } @@ -369,7 +369,7 @@ Test case is disabled by omitting the i18n attribute. const trimmed = extractMessages( `
Hello, World!
-
{{ abc }}
+
Hello {{ abc }}
Start {{ abc }} End
{{ first }} middle {{ end }}
@@ -380,7 +380,7 @@ Test case is disabled by omitting the i18n attribute. i18nPreserveWhitespaceForLegacyExtraction does not support trimming ICU case text. Test case is disabled by omitting the i18n attribute. -
{ +
Hello { apples, plural, =1 {One apple.} =other {Many apples.}