From 2cf3fa94a0c8e1c6ec214fc250a048a33b05a79e Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 7 Apr 2020 17:59:36 -0700 Subject: [PATCH 1/2] fix(core): handle empty translations correctly In certain use-cases it's useful to have an ability to use empty strings as translations. Currently Ivy fails at runtime if empty string is used as a translation, since some parts of internal data structures are not created properly. This commit updates runtime i18n logic to handle empty translations and avoid unnecessary extra processing for such cases. Fixes #36476. --- packages/core/src/render3/i18n.ts | 156 +++++++++++---------- packages/core/test/acceptance/i18n_spec.ts | 35 +++++ 2 files changed, 120 insertions(+), 71 deletions(-) diff --git a/packages/core/src/render3/i18n.ts b/packages/core/src/render3/i18n.ts index f947dd3942b8c..a003ba1c021b9 100644 --- a/packages/core/src/render3/i18n.ts +++ b/packages/core/src/render3/i18n.ts @@ -243,7 +243,7 @@ function removeInnerTemplateTranslation(message: string): string { * external template and removes all sub-templates. */ export function getTranslationForTemplate(message: string, subTemplateIndex?: number) { - if (typeof subTemplateIndex !== 'number') { + if (isRootTemplateMessage(subTemplateIndex)) { // We want the root template message, ignore all sub-templates return removeInnerTemplateTranslation(message); } else { @@ -409,81 +409,91 @@ function i18nStartFirstPass( const updateOpCodes: I18nUpdateOpCodes = []; const icuExpressions: TIcu[] = []; - const templateTranslation = getTranslationForTemplate(message, subTemplateIndex); - const msgParts = replaceNgsp(templateTranslation).split(PH_REGEXP); - for (let i = 0; i < msgParts.length; i++) { - let value = msgParts[i]; - if (i & 1) { - // Odd indexes are placeholders (elements and sub-templates) - if (value.charAt(0) === '/') { - // It is a closing tag - if (value.charAt(1) === TagType.ELEMENT) { - const phIndex = parseInt(value.substr(2), 10); - parentIndex = parentIndexStack[--parentIndexPointer]; - createOpCodes.push(phIndex << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.ElementEnd); - } - } else { - const phIndex = parseInt(value.substr(1), 10); - const isElement = value.charAt(0) === TagType.ELEMENT; - // The value represents a placeholder that we move to the designated index. - // Note: positive indicies indicate that a TNode with a given index should also be marked as - // parent while executing `Select` instruction. - createOpCodes.push( - (isElement ? phIndex : ~phIndex) << I18nMutateOpCode.SHIFT_REF | - I18nMutateOpCode.Select, - parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); - - if (isElement) { - parentIndexStack[++parentIndexPointer] = parentIndex = phIndex; - } - } - } else { - // Even indexes are text (including bindings & ICU expressions) - const parts = extractParts(value); - for (let j = 0; j < parts.length; j++) { - if (j & 1) { - // Odd indexes are ICU expressions - const icuExpression = parts[j] as IcuExpression; - - // Verify that ICU expression has the right shape. Translations might contain invalid - // constructions (while original messages were correct), so ICU parsing at runtime may not - // succeed (thus `icuExpression` remains a string). - if (typeof icuExpression !== 'object') { - throw new Error(`Unable to parse ICU expression in "${templateTranslation}" message.`); + if (message === '' && isRootTemplateMessage(subTemplateIndex)) { + // If top level translation is an empty string, do not invoke additional processing + // and just create op codes for empty text node instead. + const textNodeIndex = startIndex + i18nVarsCount++; + createOpCodes.push( + message, textNodeIndex, + parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); + } else { + const templateTranslation = getTranslationForTemplate(message, subTemplateIndex); + const msgParts = replaceNgsp(templateTranslation).split(PH_REGEXP); + for (let i = 0; i < msgParts.length; i++) { + let value = msgParts[i]; + if (i & 1) { + // Odd indexes are placeholders (elements and sub-templates) + if (value.charAt(0) === '/') { + // It is a closing tag + if (value.charAt(1) === TagType.ELEMENT) { + const phIndex = parseInt(value.substr(2), 10); + parentIndex = parentIndexStack[--parentIndexPointer]; + createOpCodes.push(phIndex << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.ElementEnd); } - - // Create the comment node that will anchor the ICU expression - const icuNodeIndex = startIndex + i18nVarsCount++; + } else { + const phIndex = parseInt(value.substr(1), 10); + const isElement = value.charAt(0) === TagType.ELEMENT; + // The value represents a placeholder that we move to the designated index. + // Note: positive indicies indicate that a TNode with a given index should also be marked + // as parent while executing `Select` instruction. createOpCodes.push( - COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '', icuNodeIndex, + (isElement ? phIndex : ~phIndex) << I18nMutateOpCode.SHIFT_REF | + I18nMutateOpCode.Select, parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); - // Update codes for the ICU expression - const mask = getBindingMask(icuExpression); - icuStart(icuExpressions, icuExpression, icuNodeIndex, icuNodeIndex); - // Since this is recursive, the last TIcu that was pushed is the one we want - const tIcuIndex = icuExpressions.length - 1; - updateOpCodes.push( - toMaskBit(icuExpression.mainBinding), // mask of the main binding - 3, // skip 3 opCodes if not changed - -1 - icuExpression.mainBinding, - icuNodeIndex << I18nUpdateOpCode.SHIFT_REF | I18nUpdateOpCode.IcuSwitch, tIcuIndex, - mask, // mask of all the bindings of this ICU expression - 2, // skip 2 opCodes if not changed - icuNodeIndex << I18nUpdateOpCode.SHIFT_REF | I18nUpdateOpCode.IcuUpdate, tIcuIndex); - } else if (parts[j] !== '') { - const text = parts[j] as string; - // Even indexes are text (including bindings) - const hasBinding = text.match(BINDING_REGEXP); - // Create text nodes - const textNodeIndex = startIndex + i18nVarsCount++; - createOpCodes.push( - // If there is a binding, the value will be set during update - hasBinding ? '' : text, textNodeIndex, - parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); + if (isElement) { + parentIndexStack[++parentIndexPointer] = parentIndex = phIndex; + } + } + } else { + // Even indexes are text (including bindings & ICU expressions) + const parts = extractParts(value); + for (let j = 0; j < parts.length; j++) { + if (j & 1) { + // Odd indexes are ICU expressions + const icuExpression = parts[j] as IcuExpression; + + // Verify that ICU expression has the right shape. Translations might contain invalid + // constructions (while original messages were correct), so ICU parsing at runtime may + // not succeed (thus `icuExpression` remains a string). + if (typeof icuExpression !== 'object') { + throw new Error( + `Unable to parse ICU expression in "${templateTranslation}" message.`); + } - if (hasBinding) { - addAllToArray(generateBindingUpdateOpCodes(text, textNodeIndex), updateOpCodes); + // Create the comment node that will anchor the ICU expression + const icuNodeIndex = startIndex + i18nVarsCount++; + createOpCodes.push( + COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '', icuNodeIndex, + parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); + + // Update codes for the ICU expression + const mask = getBindingMask(icuExpression); + icuStart(icuExpressions, icuExpression, icuNodeIndex, icuNodeIndex); + // Since this is recursive, the last TIcu that was pushed is the one we want + const tIcuIndex = icuExpressions.length - 1; + updateOpCodes.push( + toMaskBit(icuExpression.mainBinding), // mask of the main binding + 3, // skip 3 opCodes if not changed + -1 - icuExpression.mainBinding, + icuNodeIndex << I18nUpdateOpCode.SHIFT_REF | I18nUpdateOpCode.IcuSwitch, tIcuIndex, + mask, // mask of all the bindings of this ICU expression + 2, // skip 2 opCodes if not changed + icuNodeIndex << I18nUpdateOpCode.SHIFT_REF | I18nUpdateOpCode.IcuUpdate, tIcuIndex); + } else if (parts[j] !== '') { + const text = parts[j] as string; + // Even indexes are text (including bindings) + const hasBinding = text.match(BINDING_REGEXP); + // Create text nodes + const textNodeIndex = startIndex + i18nVarsCount++; + createOpCodes.push( + // If there is a binding, the value will be set during update + hasBinding ? '' : text, textNodeIndex, + parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); + + if (hasBinding) { + addAllToArray(generateBindingUpdateOpCodes(text, textNodeIndex), updateOpCodes); + } } } } @@ -558,6 +568,10 @@ function appendI18nNode( return tNode; } +function isRootTemplateMessage(subTemplateIndex: number|undefined): subTemplateIndex is undefined { + return subTemplateIndex === undefined; +} + /** * Handles message string post-processing for internationalization. * diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index 846cb6b0d02f9..6fd2e8e631b5b 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/packages/core/test/acceptance/i18n_spec.ts @@ -1611,6 +1611,41 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { }); }); + describe('empty translations', () => { + it('should replace existing text content with empty translation', () => { + loadTranslations({[computeMsgId('Some Text')]: ''}); + const fixture = initWithTemplate(AppComp, '
Some Text
'); + expect(fixture.nativeElement.textContent).toBe(''); + }); + + it('should replace existing DOM elements with empty translation', () => { + loadTranslations({ + [computeMsgId( + ' Start {$START_TAG_DIV}DIV{$CLOSE_TAG_DIV}' + + '{$START_TAG_SPAN}SPAN{$CLOSE_TAG_SPAN} End ')]: '', + }); + const fixture = initWithTemplate(AppComp, ` +
+ Start +
DIV
+ SPAN + End +
+ `); + expect(fixture.nativeElement.textContent).toBe(''); + }); + + it('should replace existing ICU content with empty translation', () => { + loadTranslations({ + [computeMsgId('{VAR_PLURAL, plural, =0 {zero} other {more than zero}}')]: '', + }); + const fixture = initWithTemplate(AppComp, ` +
{count, plural, =0 {zero} other {more than zero}}
+ `); + expect(fixture.nativeElement.textContent).toBe(''); + }); + }); + it('should work with directives and host bindings', () => { let directiveInstances: ClsDir[] = []; From 512f5a86dfba7710e8a0b35da79f9ce223ee9ce4 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Thu, 9 Apr 2020 16:24:38 -0700 Subject: [PATCH 2/2] fixup! fix(core): handle empty translations correctly --- packages/core/src/render3/i18n.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/core/src/render3/i18n.ts b/packages/core/src/render3/i18n.ts index a003ba1c021b9..b8f10aeff1272 100644 --- a/packages/core/src/render3/i18n.ts +++ b/packages/core/src/render3/i18n.ts @@ -376,6 +376,10 @@ export function ɵɵi18nStart(index: number, message: string, subTemplateIndex?: // This is reset to 0 when `i18nStartFirstPass` is called. let i18nVarsCount: number; +function allocNodeIndex(startIndex: number): number { + return startIndex + i18nVarsCount++; +} + /** * See `i18nStart` above. */ @@ -412,9 +416,8 @@ function i18nStartFirstPass( if (message === '' && isRootTemplateMessage(subTemplateIndex)) { // If top level translation is an empty string, do not invoke additional processing // and just create op codes for empty text node instead. - const textNodeIndex = startIndex + i18nVarsCount++; createOpCodes.push( - message, textNodeIndex, + message, allocNodeIndex(startIndex), parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); } else { const templateTranslation = getTranslationForTemplate(message, subTemplateIndex); @@ -462,7 +465,7 @@ function i18nStartFirstPass( } // Create the comment node that will anchor the ICU expression - const icuNodeIndex = startIndex + i18nVarsCount++; + const icuNodeIndex = allocNodeIndex(startIndex); createOpCodes.push( COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '', icuNodeIndex, parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); @@ -485,7 +488,7 @@ function i18nStartFirstPass( // Even indexes are text (including bindings) const hasBinding = text.match(BINDING_REGEXP); // Create text nodes - const textNodeIndex = startIndex + i18nVarsCount++; + const textNodeIndex = allocNodeIndex(startIndex); createOpCodes.push( // If there is a binding, the value will be set during update hasBinding ? '' : text, textNodeIndex,