Skip to content

Commit

Permalink
refactor(compiler): Fix some issues with i18n expressions in ICUs (an…
Browse files Browse the repository at this point in the history
…gular#52698)

We were previously counting the i18n expression index and deciding when
to apply i18n expressions based on the i18n context. These should be
done based on the i18n block instead.

PR Close angular#52698
  • Loading branch information
mmalerba authored and thePunderWoman committed Nov 10, 2023
1 parent 0864dbe commit 2c3b6c6
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,7 @@
"verifyUniqueConsts"
]
}
],
"skipForTemplatePipeline": true
]
},
{
"description": "should handle multiple icus that share same placeholder",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,19 @@ import {CompilationJob} from '../compilation';
* Adds apply operations after i18n expressions.
*/
export function applyI18nExpressions(job: CompilationJob): void {
const i18nContexts = new Map<ir.XrefId, ir.I18nContextOp>();
for (const unit of job.units) {
for (const op of unit.create) {
if (op.kind === ir.OpKind.I18nContext) {
i18nContexts.set(op.xref, op);
}
}
}

for (const unit of job.units) {
for (const op of unit.update) {
// Only add apply after expressions that are not followed by more expressions.
if (op.kind === ir.OpKind.I18nExpression && needsApplication(op)) {
if (op.kind === ir.OpKind.I18nExpression && needsApplication(i18nContexts, op)) {
// TODO: what should be the source span for the apply op?
ir.OpList.insertAfter<ir.UpdateOp>(ir.createI18nApplyOp(op.target, op.handle, null!), op);
}
Expand All @@ -27,13 +36,15 @@ export function applyI18nExpressions(job: CompilationJob): void {
/**
* Checks whether the given expression op needs to be followed with an apply op.
*/
function needsApplication(op: ir.I18nExpressionOp) {
function needsApplication(i18nContexts: Map<ir.XrefId, ir.I18nContextOp>, op: ir.I18nExpressionOp) {
// If the next op is not another expression, we need to apply.
if (op.next?.kind !== ir.OpKind.I18nExpression) {
return true;
}
// If the next op is an expression targeting a different i18n context, we need to apply.
if (op.next.context !== op.context) {
// If the next op is an expression targeting a different i18n block, we need to apply.
const context = i18nContexts.get(op.context)!;
const nextContext = i18nContexts.get(op.next.context)!;
if (context.i18nBlock !== nextContext.i18nBlock) {
return true;
}
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ export function resolveI18nExpressionPlaceholders(job: ComponentCompilationJob)
}
}

// Keep track of the next available expression index per i18n context.
// Keep track of the next available expression index per i18n block.
const expressionIndices = new Map<ir.XrefId, number>();

for (const unit of job.units) {
for (const op of unit.update) {
if (op.kind === ir.OpKind.I18nExpression) {
const index = expressionIndices.get(op.context) || 0;
const i18nContext = i18nContexts.get(op.context)!;
const index = expressionIndices.get(i18nContext.i18nBlock) || 0;
const subTemplateIndex = subTemplateIndicies.get(i18nContext.i18nBlock)!;
// Add the expression index in the appropriate params map.
const params = op.resolutionTime === ir.I18nParamResolutionTime.Creation ?
Expand All @@ -50,7 +50,7 @@ export function resolveI18nExpressionPlaceholders(job: ComponentCompilationJob)
});
params.set(op.i18nPlaceholder, values);

expressionIndices.set(op.context, index + 1);
expressionIndices.set(i18nContext.i18nBlock, index + 1);
}
}
}
Expand Down

0 comments on commit 2c3b6c6

Please sign in to comment.