From ba4340875ac8e338ff1390fc7897eecc704ef7c5 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 (#58154) 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. PR Close #58154 --- .../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 e7844d033a8..7002873e0db 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 af6a66d4f18..aea01bd5489 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 a515826ccb2..db479b2b768 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.}