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.
angular · dgp1130 · Oct 11, 2024 · Oct 11, 2024 · 293ead9407033e5560e4268017af93771a603826
commit 293ead9407033e5560e4268017af93771a603826
diff --git 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 && !(<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 `<div i18n-title title="">`.
+  private _isEmptyAttributeValue(ast: html.Node[]): boolean {
+    if (!isAttrNode(ast)) return false;
+    const node = ast[0];
+
+    return node.value.trim() === '';
+  }
+
+  // Check for cases like `<div i18n-title title="{{ name }}">`.
+  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 `<div i18n>{{ name }}</div>`.
+  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
@@ -100,6 +100,10 @@ describe('Extractor', () => {
       expect(extract('<div i18n="m|d"></div>')).toEqual([]);
     });
 
+    it('should not create a message for placeholder-only elements', () => {
+      expect(extract('<div i18n="m|d">{{ foo }}</div>')).toEqual([]);
+    });
+
     it('should ignore implicit elements in translatable elements', () => {
       expect(extract('<div i18n="m|d"><p></p></div>', ['p'])).toEqual([
         [['<ph tag name="START_PARAGRAPH"></ph name="CLOSE_PARAGRAPH">'], 'm', 'd', ''],
@@ -406,6 +410,10 @@ describe('Extractor', () => {
     it('should not create a message for empty attributes', () => {
       expect(extract('<div i18n-title="m|d" title></div>')).toEqual([]);
     });
+
+    it('should not create a message for placeholder-only attributes', () => {
+      expect(extract('<div i18n-title="m|d" title="{{ foo }}"></div>')).toEqual([]);
+    });
   });
 
   describe('implicit elements', () => {

diff --git 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(
         `
 <div i18n>Hello, World!</div>
-<div i18n>{{ abc }}</div>
+<div i18n>Hello {{ abc }}</div>
 <div i18n>Start {{ abc }} End</div>
 <div i18n>{{ first }} middle {{ end }}</div>
 <div i18n><a href="/foo">First Second</a></div>
@@ -41,7 +41,7 @@ Test case is disabled by omitting the i18n attribute.
   Hello, World!
 </div>
 <div i18n>
-  {{ abc }}
+  Hello {{ abc }}
 </div>
 <div i18n>
   Start {{ abc }} End
@@ -111,7 +111,7 @@ Test case is disabled by omitting the i18n attribute.
   Hello, World!
 </div>
 <div i18n>
-  {{ abc }}
+  Hello {{ abc }}
 </div>
 <div i18n>
   Start {{ abc }} End
@@ -171,7 +171,7 @@ Test case is disabled by omitting the i18n attribute.
     Hello, World!
   </div>
   <div i18n>
-    {{ abc }}
+    Hello {{ abc }}
   </div>
   <div i18n>
     Start {{ abc }} End
@@ -237,7 +237,7 @@ Test case is disabled by omitting the i18n attribute.
   exceeds line length.
 </div>
 <div i18n>
-  {{ veryLongExpressionWhichMaybeExceedsLineLength | async }}
+  Hello {{ veryLongExpressionWhichMaybeExceedsLineLength | async }}
 </div>
 <div i18n>
   This is a long {{ abc }} which maybe
@@ -280,7 +280,7 @@ Test case is disabled by omitting the i18n attribute.
   maybe exceeds line length.
 </div>
 <div i18n>
-  {{
+  Hello {{
     veryLongExpressionWhichMaybeExceedsLineLength
     | async
   }}
@@ -346,7 +346,7 @@ Test case is disabled by omitting the i18n attribute.
       const initial = extractMessages(
         `
 <div i18n> Hello, World! </div>
-<div i18n> {{ abc }} </div>
+<div i18n> Hello {{ abc }} </div>
 <div i18n> Start {{ abc }} End </div>
 <div i18n> {{ first }} middle {{ end }} </div>
 <div i18n> <a href="/foo">Foo</a> </div>
@@ -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.
-<div>{
+<div>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(
         `
 <div i18n>Hello, World!</div>
-<div i18n>{{ abc }}</div>
+<div i18n>Hello {{ abc }}</div>
 <div i18n>Start {{ abc }} End</div>
 <div i18n>{{ first }} middle {{ end }}</div>
 <div i18n><a href="/foo">Foo</a></div>
@@ -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.
-<div>{
+<div>Hello {
   apples, plural,
   =1 {One apple.}
   =other {Many apples.}