refactor(compiler): Handle trailing spaces in ICU placeholders (#52698)

mmalerba · thePunderWoman · commit 38aab6aa9d89 · 2023-11-10T17:01:07.000Z
In some cases ICU expression placeholders may have trailing spaces that need to be trimmed when matching the placeholder to its corresponding text binding. PR Close #52698
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/TEST_CASES.json
@@ -222,8 +222,7 @@
             "verifyUniqueConsts"
           ]
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     },
     {
       "description": "should produce proper messages when `select` or `plural` keywords have spaces after them",
@@ -237,8 +236,7 @@
             "verifyUniqueConsts"
           ]
         }
-      ],
-      "skipForTemplatePipeline": true
+      ]
     }
   ]
 }
diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts
@@ -492,7 +492,7 @@ function ingestIcu(unit: ViewCompilationUnit, icu: t.Icu) {
     const xref = unit.job.allocateXrefId();
     const icuNode = icu.i18n.nodes[0];
     unit.create.push(ir.createIcuStartOp(xref, icu.i18n, icuFromI18nMessage(icu.i18n).name, null!));
-    const {expressionPlaceholder} = icuNode;
+    const expressionPlaceholder = icuNode.expressionPlaceholder?.trimEnd();
     if (expressionPlaceholder === undefined || icu.vars[expressionPlaceholder] === undefined) {
       throw Error('ICU should have a text binding');
     }
diff --git a/packages/compiler/src/template/pipeline/src/phases/i18n_const_collection.ts b/packages/compiler/src/template/pipeline/src/phases/i18n_const_collection.ts
@@ -204,12 +204,14 @@ function i18nGenerateClosureVar(
  * Asserts that all of the message's placeholders have values.
  */
 function assertAllParamsResolved(op: ir.I18nMessageOp): asserts op is ir.I18nMessageOp {
-  for (const placeholder in op.message.placeholders) {
+  for (let placeholder in op.message.placeholders) {
+    placeholder = placeholder.trimEnd();
     if (!op.params.has(placeholder) && !op.postprocessingParams.has(placeholder)) {
       throw Error(`Failed to resolve i18n placeholder: ${placeholder}`);
     }
   }
-  for (const placeholder in op.message.placeholderToMessage) {
+  for (let placeholder in op.message.placeholderToMessage) {
+    placeholder = placeholder.trimEnd();
     if (!op.params.has(placeholder) && !op.postprocessingParams.has(placeholder)) {
       throw Error(`Failed to resolve i18n message placeholder: ${placeholder}`);
     }

Original file line number	Diff line number	Diff line change
`@@ -222,8 +222,7 @@`
`222`	`222`	`"verifyUniqueConsts"`
`223`	`223`	`]`
`224`	`224`	`}`
`225`		`- ],`
`226`		`- "skipForTemplatePipeline": true`
	`225`	`+ ]`
`227`	`226`	`},`
`228`	`227`	`{`
`229`	`228`	"description": "should produce proper messages when `select` or `plural` keywords have spaces after them",
`@@ -237,8 +236,7 @@`
`237`	`236`	`"verifyUniqueConsts"`
`238`	`237`	`]`
`239`	`238`	`}`
`240`		`- ],`
`241`		`- "skipForTemplatePipeline": true`
	`239`	`+ ]`
`242`	`240`	`}`
`243`	`241`	`]`
`244`	`242`	`}`
Original file line number	Diff line number	Diff line change
`@@ -492,7 +492,7 @@ function ingestIcu(unit: ViewCompilationUnit, icu: t.Icu) {`
`492`	`492`	`const xref = unit.job.allocateXrefId();`
`493`	`493`	`const icuNode = icu.i18n.nodes[0];`
`494`	`494`	`unit.create.push(ir.createIcuStartOp(xref, icu.i18n, icuFromI18nMessage(icu.i18n).name, null!));`
`495`		`- const {expressionPlaceholder} = icuNode;`
	`495`	`+ const expressionPlaceholder = icuNode.expressionPlaceholder?.trimEnd();`
`496`	`496`	`if (expressionPlaceholder === undefined \|\| icu.vars[expressionPlaceholder] === undefined) {`
`497`	`497`	`throw Error('ICU should have a text binding');`
`498`	`498`	`}`
Original file line number	Diff line number	Diff line change
`@@ -204,12 +204,14 @@ function i18nGenerateClosureVar(`
`204`	`204`	`* Asserts that all of the message's placeholders have values.`
`205`	`205`	`*/`
`206`	`206`	`function assertAllParamsResolved(op: ir.I18nMessageOp): asserts op is ir.I18nMessageOp {`
`207`		`- for (const placeholder in op.message.placeholders) {`
	`207`	`+ for (let placeholder in op.message.placeholders) {`
	`208`	`+ placeholder = placeholder.trimEnd();`
`208`	`209`	`if (!op.params.has(placeholder) && !op.postprocessingParams.has(placeholder)) {`
`209`	`210`	throw Error(`Failed to resolve i18n placeholder: ${placeholder}`);
`210`	`211`	`}`
`211`	`212`	`}`
`212`		`- for (const placeholder in op.message.placeholderToMessage) {`
	`213`	`+ for (let placeholder in op.message.placeholderToMessage) {`
	`214`	`+ placeholder = placeholder.trimEnd();`
`213`	`215`	`if (!op.params.has(placeholder) && !op.postprocessingParams.has(placeholder)) {`
`214`	`216`	throw Error(`Failed to resolve i18n message placeholder: ${placeholder}`);
`215`	`217`	`}`