Skip to content

Commit

Permalink
fix(core): move generated i18n statements to the consts field of Co…
Browse files Browse the repository at this point in the history
…mponentDef (angular#38404)

This commit updates the code to move generated i18n statements into the `consts` field of
ComponentDef to avoid invoking `$localize` function before component initialization (to better
support runtime translations) and also avoid problems with lazy-loading when i18n defs may not
be present in a chunk where it's referenced.

Prior to this change the i18n statements were generated at the top leve:

```
var I18N_0;
if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) {
    var MSG_X = goog.getMsg(“…”);
    I18N_0 = MSG_X;
} else {
    I18N_0 = $localize('...');
}

defineComponent({
    // ...
    template: function App_Template(rf, ctx) {
        i0.ɵɵi18n(2, I18N_0);
    }
});
```

This commit updates the logic to generate the following code instead:

```
defineComponent({
    // ...
    consts: function() {
        var I18N_0;
        if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) {
            var MSG_X = goog.getMsg(“…”);
            I18N_0 = MSG_X;
        } else {
            I18N_0 = $localize('...');
        }
        return [
            I18N_0
        ];
    },
    template: function App_Template(rf, ctx) {
        i0.ɵɵi18n(2, 0);
    }
});
```

Also note that i18n template instructions now refer to the `consts` array using an index
(similar to other template instructions).

PR Close angular#38404
  • Loading branch information
AndrewKushnir authored and atscott committed Aug 17, 2020
1 parent 5f90b64 commit cb05c01
Show file tree
Hide file tree
Showing 10 changed files with 751 additions and 449 deletions.
960 changes: 610 additions & 350 deletions packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts

Large diffs are not rendered by default.

17 changes: 13 additions & 4 deletions packages/compiler/src/render3/view/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,19 @@ export function compileComponentFromMetadata(
// e.g. `vars: 2`
definitionMap.set('vars', o.literal(templateBuilder.getVarCount()));

// e.g. `consts: [['one', 'two'], ['three', 'four']]
const consts = templateBuilder.getConsts();
if (consts.length > 0) {
definitionMap.set('consts', o.literalArr(consts));
// Generate `consts` section of ComponentDef:
// - either as an array:
// `consts: [['one', 'two'], ['three', 'four']]`
// - or as a factory function in case additional statements are present (to support i18n):
// `consts: function() { var i18n_0; if (ngI18nClosureMode) {...} else {...} return [i18n_0]; }`
const {constExpressions, prepareStatements} = templateBuilder.getConsts();
if (constExpressions.length > 0) {
let constsExpr: o.LiteralArrayExpr|o.FunctionExpr = o.literalArr(constExpressions);
// Prepare statements are present - turn `consts` into a function.
if (prepareStatements.length > 0) {
constsExpr = o.fn([], [...prepareStatements, new o.ReturnStatement(constsExpr)]);
}
definitionMap.set('consts', constsExpr);
}

definitionMap.set('template', templateFunctionExpression);
Expand Down
12 changes: 8 additions & 4 deletions packages/compiler/src/render3/view/i18n/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ import * as o from '../../../output/output_ast';
import * as t from '../../r3_ast';

/* Closure variables holding messages must be named `MSG_[A-Z0-9]+` */
const CLOSURE_TRANSLATION_PREFIX = 'MSG_';
const CLOSURE_TRANSLATION_VAR_PREFIX = 'MSG_';

/* Prefix for non-`goog.getMsg` i18n-related vars */
export const TRANSLATION_PREFIX = 'I18N_';
/**
* Prefix for non-`goog.getMsg` i18n-related vars.
* Note: the prefix uses lowercase characters intentionally due to a Closure behavior that
* considers variables like `I18N_0` as constants and throws an error when their value changes.
*/
export const TRANSLATION_VAR_PREFIX = 'i18n_';

/** Name of the i18n attributes **/
export const I18N_ATTR = 'i18n';
Expand Down Expand Up @@ -166,7 +170,7 @@ export function formatI18nPlaceholderName(name: string, useCamelCase: boolean =
* @returns Complete translation const prefix
*/
export function getTranslationConstPrefix(extra: string): string {
return `${CLOSURE_TRANSLATION_PREFIX}${extra}`.toUpperCase();
return `${CLOSURE_TRANSLATION_VAR_PREFIX}${extra}`.toUpperCase();
}

/**
Expand Down
52 changes: 35 additions & 17 deletions packages/compiler/src/render3/view/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {I18nContext} from './i18n/context';
import {createGoogleGetMsgStatements} from './i18n/get_msg_utils';
import {createLocalizeStatements} from './i18n/localize_utils';
import {I18nMetaVisitor} from './i18n/meta';
import {assembleBoundTextPlaceholders, assembleI18nBoundString, declareI18nVariable, getTranslationConstPrefix, hasI18nMeta, I18N_ICU_MAPPING_PREFIX, i18nFormatPlaceholderNames, icuFromI18nMessage, isI18nRootNode, isSingleI18nIcu, placeholdersToParams, TRANSLATION_PREFIX, wrapI18nPlaceholder} from './i18n/util';
import {assembleBoundTextPlaceholders, assembleI18nBoundString, declareI18nVariable, getTranslationConstPrefix, hasI18nMeta, I18N_ICU_MAPPING_PREFIX, i18nFormatPlaceholderNames, icuFromI18nMessage, isI18nRootNode, isSingleI18nIcu, placeholdersToParams, TRANSLATION_VAR_PREFIX, wrapI18nPlaceholder} from './i18n/util';
import {StylingBuilder, StylingInstruction} from './styling_builder';
import {asLiteral, chainedInstruction, CONTEXT_NAME, getAttrsForDirectiveMatching, getInterpolationArgsLength, IMPLICIT_REFERENCE, invalid, NON_BINDABLE_ATTR, REFERENCE_PREFIX, RENDER_FLAGS, trimTrailingNulls, unsupported} from './util';

Expand Down Expand Up @@ -103,6 +103,18 @@ export function prepareEventListenerParameters(
return params;
}

// Collects information needed to generate `consts` field of the ComponentDef.
// When a constant requires some pre-processing, the `prepareStatements` section
// contains corresponding statements.
export interface ComponentDefConsts {
prepareStatements: o.Statement[];
constExpressions: o.Expression[];
}

function createComponentDefConsts(): ComponentDefConsts {
return {prepareStatements: [], constExpressions: []};
}

export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver {
private _dataIndex = 0;
private _bindingContext = 0;
Expand Down Expand Up @@ -171,7 +183,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
private directiveMatcher: SelectorMatcher|null, private directives: Set<o.Expression>,
private pipeTypeByName: Map<string, o.Expression>, private pipes: Set<o.Expression>,
private _namespace: o.ExternalReference, relativeContextFilePath: string,
private i18nUseExternalIds: boolean, private _constants: o.Expression[] = []) {
private i18nUseExternalIds: boolean,
private _constants: ComponentDefConsts = createComponentDefConsts()) {
this._bindingScope = parentBindingScope.nestedScope(level);

// Turn the relative context file path into an identifier by replacing non-alphanumeric
Expand Down Expand Up @@ -307,12 +320,12 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
private i18nTranslate(
message: i18n.Message, params: {[name: string]: o.Expression} = {}, ref?: o.ReadVarExpr,
transformFn?: (raw: o.ReadVarExpr) => o.Expression): o.ReadVarExpr {
const _ref = ref || o.variable(this.constantPool.uniqueName(TRANSLATION_PREFIX));
const _ref = ref || this.i18nGenerateMainBlockVar();
// Closure Compiler requires const names to start with `MSG_` but disallows any other const to
// start with `MSG_`. We define a variable starting with `MSG_` just for the `goog.getMsg` call
const closureVar = this.i18nGenerateClosureVar(message.id);
const statements = getTranslationDeclStmts(message, _ref, closureVar, params, transformFn);
this.constantPool.statements.push(...statements);
this._constants.prepareStatements.push(...statements);
return _ref;
}

Expand Down Expand Up @@ -364,6 +377,12 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
return bound;
}

// Generates top level vars for i18n blocks (i.e. `i18n_N`).
private i18nGenerateMainBlockVar(): o.ReadVarExpr {
return o.variable(this.constantPool.uniqueName(TRANSLATION_VAR_PREFIX));
}

// Generates vars with Closure-specific names for i18n blocks (i.e. `MSG_XXX`).
private i18nGenerateClosureVar(messageId: string): o.ReadVarExpr {
let name: string;
const suffix = this.fileBasedI18nSuffix.toUpperCase();
Expand Down Expand Up @@ -426,16 +445,13 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
private i18nStart(span: ParseSourceSpan|null = null, meta: i18n.I18nMeta, selfClosing?: boolean):
void {
const index = this.allocateDataSlot();
if (this.i18nContext) {
this.i18n = this.i18nContext.forkChildContext(index, this.templateIndex!, meta);
} else {
const ref = o.variable(this.constantPool.uniqueName(TRANSLATION_PREFIX));
this.i18n = new I18nContext(index, ref, 0, this.templateIndex, meta);
}
this.i18n = this.i18nContext ?
this.i18nContext.forkChildContext(index, this.templateIndex!, meta) :
new I18nContext(index, this.i18nGenerateMainBlockVar(), 0, this.templateIndex, meta);

// generate i18nStart instruction
const {id, ref} = this.i18n;
const params: o.Expression[] = [o.literal(index), ref];
const params: o.Expression[] = [o.literal(index), this.addToConsts(ref)];
if (id > 0) {
// do not push 3rd argument (sub-block id)
// into i18nStart call for top level i18n context
Expand Down Expand Up @@ -507,8 +523,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
}
if (i18nAttrArgs.length > 0) {
const index: o.Expression = o.literal(this.allocateDataSlot());
const args = this.constantPool.getConstLiteral(o.literalArr(i18nAttrArgs), true);
this.creationInstruction(sourceSpan, R3.i18nAttributes, [index, args]);
const constIndex = this.addToConsts(o.literalArr(i18nAttrArgs));
this.creationInstruction(sourceSpan, R3.i18nAttributes, [index, constIndex]);
if (hasBindings) {
this.updateInstruction(sourceSpan, R3.i18nApply, [index]);
}
Expand Down Expand Up @@ -1028,7 +1044,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
return this._pureFunctionSlots;
}

getConsts() {
getConsts(): ComponentDefConsts {
return this._constants;
}

Expand Down Expand Up @@ -1352,14 +1368,16 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
return o.TYPED_NULL_EXPR;
}

const consts = this._constants.constExpressions;

// Try to reuse a literal that's already in the array, if possible.
for (let i = 0; i < this._constants.length; i++) {
if (this._constants[i].isEquivalent(expression)) {
for (let i = 0; i < consts.length; i++) {
if (consts[i].isEquivalent(expression)) {
return o.literal(i);
}
}

return o.literal(this._constants.push(expression) - 1);
return o.literal(consts.push(expression) - 1);
}

private addAttrsToConsts(attrs: o.Expression[]): o.LiteralExpr {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/render3/definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {stringify} from '../util/stringify';
import {EMPTY_ARRAY, EMPTY_OBJ} from './empty';
import {NG_COMP_DEF, NG_DIR_DEF, NG_FACTORY_DEF, NG_LOC_ID_DEF, NG_MOD_DEF, NG_PIPE_DEF} from './fields';
import {ComponentDef, ComponentDefFeature, ComponentTemplate, ComponentType, ContentQueriesFunction, DirectiveDef, DirectiveDefFeature, DirectiveTypesOrFactory, FactoryFn, HostBindingsFunction, PipeDef, PipeType, PipeTypesOrFactory, ViewQueriesFunction} from './interfaces/definition';
import {AttributeMarker, TAttributes, TConstants} from './interfaces/node';
import {AttributeMarker, TAttributes, TConstantsOrFactory} from './interfaces/node';
import {CssSelectorList, SelectorFlags} from './interfaces/projection';
import {NgModuleType} from './ng_module_ref';

Expand Down Expand Up @@ -220,7 +220,7 @@ export function ɵɵdefineComponent<T>(componentDefinition: {
* Constants for the nodes in the component's view.
* Includes attribute arrays, local definition arrays etc.
*/
consts?: TConstants;
consts?: TConstantsOrFactory;

/**
* An array of `ngContent[selector]` values that were found in the template.
Expand Down
17 changes: 10 additions & 7 deletions packages/core/src/render3/instructions/i18n.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {i18nAttributesFirstPass, i18nStartFirstPass} from '../i18n/i18n_parse';
import {i18nPostprocess} from '../i18n/i18n_postprocess';
import {HEADER_OFFSET} from '../interfaces/view';
import {getLView, getTView, nextBindingIndex} from '../state';
import {getConstant} from '../util/view_utils';

import {setDelayProjection} from './all';

Expand Down Expand Up @@ -42,14 +43,15 @@ import {setDelayProjection} from './all';
* `template` instruction index. A `block` that matches the sub-template in which it was declared.
*
* @param index A unique index of the translation in the static block.
* @param message The translation message.
* @param messageIndex An index of the translation message from the `def.consts` array.
* @param subTemplateIndex Optional sub-template index in the `message`.
*
* @codeGenApi
*/
export function ɵɵi18nStart(index: number, message: string, subTemplateIndex?: number): void {
export function ɵɵi18nStart(index: number, messageIndex: number, subTemplateIndex?: number): void {
const tView = getTView();
ngDevMode && assertDefined(tView, `tView should be defined`);
const message = getConstant<string>(tView.consts, messageIndex)!;
pushI18nIndex(index);
// We need to delay projections until `i18nEnd`
setDelayProjection(true);
Expand Down Expand Up @@ -96,13 +98,13 @@ export function ɵɵi18nEnd(): void {
* `template` instruction index. A `block` that matches the sub-template in which it was declared.
*
* @param index A unique index of the translation in the static block.
* @param message The translation message.
* @param messageIndex An index of the translation message from the `def.consts` array.
* @param subTemplateIndex Optional sub-template index in the `message`.
*
* @codeGenApi
*/
export function ɵɵi18n(index: number, message: string, subTemplateIndex?: number): void {
ɵɵi18nStart(index, message, subTemplateIndex);
export function ɵɵi18n(index: number, messageIndex: number, subTemplateIndex?: number): void {
ɵɵi18nStart(index, messageIndex, subTemplateIndex);
ɵɵi18nEnd();
}

Expand All @@ -114,11 +116,12 @@ export function ɵɵi18n(index: number, message: string, subTemplateIndex?: numb
*
* @codeGenApi
*/
export function ɵɵi18nAttributes(index: number, values: string[]): void {
export function ɵɵi18nAttributes(index: number, attrsIndex: number): void {
const lView = getLView();
const tView = getTView();
ngDevMode && assertDefined(tView, `tView should be defined`);
i18nAttributesFirstPass(lView, tView, index, values);
const attrs = getConstant<string[]>(tView.consts, attrsIndex)!;
i18nAttributesFirstPass(lView, tView, index, attrs);
}


Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} fr
import {CONTAINER_HEADER_OFFSET, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition';
import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector';
import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliases, PropertyAliasValue, TAttributes, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node';
import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliases, PropertyAliasValue, TAttributes, TConstantsOrFactory, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node';
import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, RNode, RText} from '../interfaces/renderer';
import {SanitizerFn} from '../interfaces/sanitization';
import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks';
Expand Down Expand Up @@ -650,14 +650,15 @@ export function createTView(
type: TViewType, viewIndex: number, templateFn: ComponentTemplate<any>|null, decls: number,
vars: number, directives: DirectiveDefListOrFactory|null, pipes: PipeDefListOrFactory|null,
viewQuery: ViewQueriesFunction<any>|null, schemas: SchemaMetadata[]|null,
consts: TConstants|null): TView {
constsOrFactory: TConstantsOrFactory|null): TView {
ngDevMode && ngDevMode.tView++;
const bindingStartIndex = HEADER_OFFSET + decls;
// This length does not yet contain host bindings from child directives because at this point,
// we don't know which directives are active on this template. As soon as a directive is matched
// that has a host binding, we will update the blueprint with that def's hostVars count.
const initialViewLength = bindingStartIndex + vars;
const blueprint = createViewBlueprint(bindingStartIndex, initialViewLength);
const consts = typeof constsOrFactory === 'function' ? constsOrFactory() : constsOrFactory;
const tView = blueprint[TVIEW as any] = ngDevMode ?
new TViewConstructor(
type,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/render3/interfaces/definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {SchemaMetadata, ViewEncapsulation} from '../../core';
import {ProcessProvidersFunction} from '../../di/interface/provider';
import {Type} from '../../interface/type';

import {TAttributes, TConstants} from './node';
import {TAttributes, TConstantsOrFactory} from './node';
import {CssSelectorList} from './projection';
import {TView} from './view';

Expand Down Expand Up @@ -299,7 +299,7 @@ export interface ComponentDef<T> extends DirectiveDef<T> {
readonly template: ComponentTemplate<T>;

/** Constants associated with the component's view. */
readonly consts: TConstants|null;
readonly consts: TConstantsOrFactory|null;

/**
* An array of `ngContent[selector]` values that were found in the template.
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/render3/interfaces/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,24 @@ export type TAttributes = (string|AttributeMarker|CssSelector)[];
* Constants that are associated with a view. Includes:
* - Attribute arrays.
* - Local definition arrays.
* - Translated messages (i18n).
*/
export type TConstants = (TAttributes|string)[];

/**
* Factory function that returns an array of consts. Consts can be represented as a function in case
* any additional statements are required to define consts in the list. An example is i18n where
* additional i18n calls are generated, which should be executed when consts are requested for the
* first time.
*/
export type TConstantsFactory = () => TConstants;

/**
* TConstants type that describes how the `consts` field is generated on ComponentDef: it can be
* either an array or a factory function that returns that array.
*/
export type TConstantsOrFactory = TConstants|TConstantsFactory;

/**
* Binding data (flyweight) for a particular node that is shared between all templates
* of a specific type.
Expand Down
Loading

0 comments on commit cb05c01

Please sign in to comment.