refactor(compiler): [i18n] integrate review feedback

modules/@angular/compiler/src/i18n/i18n_ast.ts

@@ -79,3 +79,56 @@ export interface Visitor {
   visitPlaceholder(ph: Placeholder, context?: any): any;
   visitIcuPlaceholder(ph: IcuPlaceholder, context?: any): any;
 }
+
+// Clone the AST
+export class CloneVisitor implements Visitor {
+  visitText(text: Text, context?: any): Text { return new Text(text.value, text.sourceSpan); }
+
+  visitContainer(container: Container, context?: any): Container {
+    const children = container.children.map(n => n.visit(this, context));
+    return new Container(children, container.sourceSpan);
+  }
+
+  visitIcu(icu: Icu, context?: any): Icu {
+    const cases: {[k: string]: Node} = {};
+    Object.keys(icu.cases).forEach(key => cases[key] = icu.cases[key].visit(this, context));
+    const msg = new Icu(icu.expression, icu.type, cases, icu.sourceSpan);
+    msg.expressionPlaceholder = icu.expressionPlaceholder;
+    return msg;
+  }
+
+  visitTagPlaceholder(ph: TagPlaceholder, context?: any): TagPlaceholder {
+    const children = ph.children.map(n => n.visit(this, context));
+    return new TagPlaceholder(
+        ph.tag, ph.attrs, ph.startName, ph.closeName, children, ph.isVoid, ph.sourceSpan);
+  }
+
+  visitPlaceholder(ph: Placeholder, context?: any): Placeholder {
+    return new Placeholder(ph.value, ph.name, ph.sourceSpan);
+  }
+
+  visitIcuPlaceholder(ph: IcuPlaceholder, context?: any): IcuPlaceholder {
+    return new IcuPlaceholder(ph.value, ph.name, ph.sourceSpan);
+  }
+}
+
+// Visit all the nodes recursively
+export class RecurseVisitor implements Visitor {
+  visitText(text: Text, context?: any): any{};
+
+  visitContainer(container: Container, context?: any): any {
+    container.children.forEach(child => child.visit(this));
+  }
+
+  visitIcu(icu: Icu, context?: any): any {
+    Object.keys(icu.cases).forEach(k => { icu.cases[k].visit(this); });
+  }
+
+  visitTagPlaceholder(ph: TagPlaceholder, context?: any): any {
+    ph.children.forEach(child => child.visit(this));
+  }
+
+  visitPlaceholder(ph: Placeholder, context?: any): any{};
+
+  visitIcuPlaceholder(ph: IcuPlaceholder, context?: any): any{};
+}

modules/@angular/compiler/src/i18n/message_bundle.ts

@@ -72,28 +72,11 @@ export class MessageBundle {
 }
 
 // Transform an i18n AST by renaming the placeholder nodes with the given mapper
-class MapPlaceholderNames implements i18n.Visitor {
+class MapPlaceholderNames extends i18n.CloneVisitor {
   convert(nodes: i18n.Node[], mapper: PlaceholderMapper): i18n.Node[] {
     return mapper ? nodes.map(n => n.visit(this, mapper)) : nodes;
   }
 
-  visitText(text: i18n.Text, mapper: PlaceholderMapper): i18n.Text {
-    return new i18n.Text(text.value, text.sourceSpan);
-  }
-
-  visitContainer(container: i18n.Container, mapper: PlaceholderMapper): i18n.Container {
-    const children = container.children.map(n => n.visit(this, mapper));
-    return new i18n.Container(children, container.sourceSpan);
-  }
-
-  visitIcu(icu: i18n.Icu, mapper: PlaceholderMapper): i18n.Icu {
-    const cases: {[k: string]: i18n.Node} = {};
-    Object.keys(icu.cases).forEach(key => cases[key] = icu.cases[key].visit(this, mapper));
-    const msg = new i18n.Icu(icu.expression, icu.type, cases, icu.sourceSpan);
-    msg.expressionPlaceholder = icu.expressionPlaceholder;
-    return msg;
-  }
-
   visitTagPlaceholder(ph: i18n.TagPlaceholder, mapper: PlaceholderMapper): i18n.TagPlaceholder {
     const startName = mapper.toPublicName(ph.startName);
     const closeName = ph.closeName ? mapper.toPublicName(ph.closeName) : ph.closeName;

modules/@angular/compiler/src/i18n/serializers/serializer.ts

@@ -34,3 +34,64 @@ export interface PlaceholderMapper {
 
   toInternalName(publicName: string): string;
 }
+
+/**
+ * A simple mapper that take a function to transform an internal name to a public name
+ */
+export class SimplePlaceholderMapper extends i18n.RecurseVisitor implements PlaceholderMapper {
+  private internalToPublic: {[k: string]: string} = {};
+  private publicToNextId: {[k: string]: number} = {};
+  private publicToInternal: {[k: string]: string} = {};
+
+  // create a mapping from the message
+  constructor(message: i18n.Message, private mapName: (name: string) => string) {
+    super();
+    message.nodes.forEach(node => node.visit(this));
+  }
+
+  toPublicName(internalName: string): string {
+    return this.internalToPublic.hasOwnProperty(internalName) ?
+        this.internalToPublic[internalName] :
+        null;
+  }
+
+  toInternalName(publicName: string): string {
+    return this.publicToInternal.hasOwnProperty(publicName) ? this.publicToInternal[publicName] :
+                                                              null;
+  }
+
+  visitText(text: i18n.Text, context?: any): any { return null; }
+
+  visitTagPlaceholder(ph: i18n.TagPlaceholder, context?: any): any {
+    this.visitPlaceholderName(ph.startName);
+    super.visitTagPlaceholder(ph, context);
+    this.visitPlaceholderName(ph.closeName);
+  }
+
+  visitPlaceholder(ph: i18n.Placeholder, context?: any): any { this.visitPlaceholderName(ph.name); }
+
+  visitIcuPlaceholder(ph: i18n.IcuPlaceholder, context?: any): any {
+    this.visitPlaceholderName(ph.name);
+  }
+
+  // XMB placeholders could only contains A-Z, 0-9 and _
+  private visitPlaceholderName(internalName: string): void {
+    if (!internalName || this.internalToPublic.hasOwnProperty(internalName)) {
+      return;
+    }
+
+    let publicName = this.mapName(internalName);
+
+    if (this.publicToInternal.hasOwnProperty(publicName)) {
+      // Create a new XMB when it has already been used
+      const nextId = this.publicToNextId[publicName];
+      this.publicToNextId[publicName] = nextId + 1;
+      publicName = `${publicName}_${nextId}`;
+    } else {
+      this.publicToNextId[publicName] = 1;
+    }
+
+    this.internalToPublic[internalName] = publicName;
+    this.publicToInternal[publicName] = internalName;
+  }
+}

modules/@angular/compiler/src/i18n/serializers/xmb.ts

@@ -9,7 +9,7 @@
 import {decimalDigest} from '../digest';
 import * as i18n from '../i18n_ast';
 
-import {PlaceholderMapper, Serializer} from './serializer';
+import {PlaceholderMapper, Serializer, SimplePlaceholderMapper} from './serializer';
 import * as xml from './xml_helper';
 
 const _MESSAGES_TAG = 'messagebundle';
@@ -78,7 +78,7 @@ export class Xmb extends Serializer {
 
 
   createNameMapper(message: i18n.Message): PlaceholderMapper {
-    return new XmbPlaceholderMapper(message);
+    return new SimplePlaceholderMapper(message, toPublicName);
   }
 }
 
@@ -157,68 +157,7 @@ class ExampleVisitor implements xml.IVisitor {
   visitDoctype(doctype: xml.Doctype): void {}
 }
 
-/**
+// XMB/XTB placeholders can only contain A-Z, 0-9 and _
- * XMB/XTB placeholders can only contain A-Z, 0-9 and _
+export function toPublicName(internalName: string): string {
- *
+  return internalName.toUpperCase().replace(/[^A-Z0-9_]/g, '_');
- * Because such restrictions do not exist on placeholder names generated locally, the
- * `PlaceholderMapper` is used to convert internal names to XMB names when the XMB file is
- * serialized and back from XTB to internal names when an XTB is loaded.
- */
-export class XmbPlaceholderMapper implements PlaceholderMapper, i18n.Visitor {
-  private internalToXmb: {[k: string]: string} = {};
-  private xmbToNextId: {[k: string]: number} = {};
-  private xmbToInternal: {[k: string]: string} = {};
-
-  // create a mapping from the message
-  constructor(message: i18n.Message) { message.nodes.forEach(node => node.visit(this)); }
-
-  toPublicName(internalName: string): string {
-    return this.internalToXmb.hasOwnProperty(internalName) ? this.internalToXmb[internalName] :
-                                                             null;
-  }
-
-  toInternalName(publicName: string): string {
-    return this.xmbToInternal.hasOwnProperty(publicName) ? this.xmbToInternal[publicName] : null;
-  }
-
-  visitText(text: i18n.Text, context?: any): any { return null; }
-
-  visitContainer(container: i18n.Container, context?: any): any {
-    container.children.forEach(child => child.visit(this));
-  }
-
-  visitIcu(icu: i18n.Icu, context?: any): any {
-    Object.keys(icu.cases).forEach(k => { icu.cases[k].visit(this); });
-  }
-
-  visitTagPlaceholder(ph: i18n.TagPlaceholder, context?: any): any {
-    this.addPlaceholder(ph.startName);
-    ph.children.forEach(child => child.visit(this));
-    this.addPlaceholder(ph.closeName);
-  }
-
-  visitPlaceholder(ph: i18n.Placeholder, context?: any): any { this.addPlaceholder(ph.name); }
-
-  visitIcuPlaceholder(ph: i18n.IcuPlaceholder, context?: any): any { this.addPlaceholder(ph.name); }
-
-  // XMB placeholders could only contains A-Z, 0-9 and _
-  private addPlaceholder(internalName: string): void {
-    if (!internalName || this.internalToXmb.hasOwnProperty(internalName)) {
-      return;
-    }
-
-    let xmbName = internalName.toUpperCase().replace(/[^A-Z0-9_]/g, '_');
-
-    if (this.xmbToInternal.hasOwnProperty(xmbName)) {
-      // Create a new XMB when it has already been used
-      const nextId = this.xmbToNextId[xmbName];
-      this.xmbToNextId[xmbName] = nextId + 1;
-      xmbName = `${xmbName}_${nextId}`;
-    } else {
-      this.xmbToNextId[xmbName] = 1;
-    }
-
-    this.internalToXmb[internalName] = xmbName;
-    this.xmbToInternal[xmbName] = internalName;
-  }
 }

modules/@angular/compiler/src/i18n/serializers/xtb.ts

@@ -11,8 +11,8 @@ import {XmlParser} from '../../ml_parser/xml_parser';
 import * as i18n from '../i18n_ast';
 import {I18nError} from '../parse_util';
 
-import {PlaceholderMapper, Serializer} from './serializer';
+import {PlaceholderMapper, Serializer, SimplePlaceholderMapper} from './serializer';
-import {XmbPlaceholderMapper, digest} from './xmb';
+import {digest, toPublicName} from './xmb';
 
 const _TRANSLATIONS_TAG = 'translationbundle';
 const _TRANSLATION_TAG = 'translation';
@@ -45,7 +45,7 @@ export class Xtb extends Serializer {
   digest(message: i18n.Message): string { return digest(message); }
 
   createNameMapper(message: i18n.Message): PlaceholderMapper {
-    return new XmbPlaceholderMapper(message);
+    return new SimplePlaceholderMapper(message, toPublicName);
   }
 }
 

modules/@angular/compiler/test/i18n/i18n_parser_spec.ts

@@ -291,7 +291,7 @@ export function main() {
   });
 }
 
-function _humanizeMessages(
+export function _humanizeMessages(
     html: string, implicitTags: string[] = [],
     implicitAttrs: {[k: string]: string[]} = {}): [string[], string, string][] {
   // clang-format off
@@ -322,7 +322,7 @@ function _humanizePlaceholdersToMessage(
 }
 
 
-function _extractMessages(
+export function _extractMessages(
     html: string, implicitTags: string[] = [],
     implicitAttrs: {[k: string]: string[]} = {}): Message[] {
   const htmlParser = new HtmlParser();

modules/@angular/compiler/test/i18n/serializers/i18n_ast_spec.ts

@@ -0,0 +1,66 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import * as i18n from '@angular/compiler/src/i18n/i18n_ast';
+
+import {serializeNodes} from '../../../src/i18n/digest';
+import {_extractMessages} from '../i18n_parser_spec';
+
+export function main(): void {
+  describe('i18n AST', () => {
+    describe('CloneVisitor', () => {
+      it('should clone an AST', () => {
+        const messages = _extractMessages(
+            '<div i18n="m|d">b{count, plural, =0 {{sex, select, male {m}}}}a</div>');
+        const nodes = messages[0].nodes;
+        const text = serializeNodes(nodes).join('');
+        expect(text).toEqual(
+            'b<ph icu name="ICU">{count, plural, =0 {[{sex, select, male {[m]}}]}}</ph>a');
+        const visitor = new i18n.CloneVisitor();
+        const cloneNodes = nodes.map(n => n.visit(visitor));
+        expect(serializeNodes(nodes)).toEqual(serializeNodes(cloneNodes));
+        nodes.forEach((n: i18n.Node, i: number) => {
+          expect(n).toEqual(cloneNodes[i]);
+          expect(n).not.toBe(cloneNodes[i]);
+        });
+      });
+    });
+
+    describe('RecurseVisitor', () => {
+      it('should visit all nodes', () => {
+        const visitor = new RecurseVisitor();
+        const container = new i18n.Container(
+            [
+              new i18n.Text('', null),
+              new i18n.Placeholder('', '', null),
+              new i18n.IcuPlaceholder(null, '', null),
+            ],
+            null);
+        const tag = new i18n.TagPlaceholder('', {}, '', '', [container], false, null);
+        const icu = new i18n.Icu('', '', {tag}, null);
+
+        icu.visit(visitor);
+        expect(visitor.textCount).toEqual(1);
+        expect(visitor.phCount).toEqual(1);
+        expect(visitor.icuPhCount).toEqual(1);
+      });
+    });
+  });
+}
+
+class RecurseVisitor extends i18n.RecurseVisitor {
+  textCount = 0;
+  phCount = 0;
+  icuPhCount = 0;
+
+  visitText(text: i18n.Text, context?: any): any { this.textCount++; }
+
+  visitPlaceholder(ph: i18n.Placeholder, context?: any): any { this.phCount++; }
+
+  visitIcuPlaceholder(ph: i18n.IcuPlaceholder, context?: any): any { this.icuPhCount++; }
+}