refactor(i18n): misc updates

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

@@ -11,9 +11,10 @@ import {I18nError} from './parse_util';
 
 const _I18N_ATTR = 'i18n';
 const _I18N_ATTR_PREFIX = 'i18n-';
+const _I18N_COMMENT_PREFIX_REGEXP = /^i18n:?/;
 
 /**
- * Extract translatable message from an html AST as a list of html AST nodes
+ * Extract translatable messages from an html AST as a list of html AST nodes
  */
 export function extractAstMessages(
     sourceAst: html.Node[], implicitTags: string[],
@@ -40,19 +41,14 @@ class _ExtractVisitor implements html.Visitor {
   // {<icu message>}
   private _inIcu = false;
 
-  private _sectionStartIndex: number;
+  private _msgCountAtSectionStart: number;
   private _errors: I18nError[];
 
   constructor(private _implicitTags: string[], private _implicitAttrs: {[k: string]: string[]}) {}
 
   extract(nodes: html.Node[]): ExtractionResult {
+    this._init();
     const messages: Message[] = [];
-    this._inI18nBlock = false;
-    this._inI18nNode = false;
-    this._depth = 0;
-    this._inIcu = false;
-    this._sectionStartIndex = void 0;
-    this._errors = [];
 
     nodes.forEach(node => node.visit(this, messages));
 
@@ -105,13 +101,13 @@ class _ExtractVisitor implements html.Visitor {
           this._inI18nBlock = true;
           this._blockStartDepth = this._depth;
           this._blockChildren = [];
-          this._blockMeaningAndDesc = comment.value.replace(/^i18n:?/, '').trim();
+          this._blockMeaningAndDesc = comment.value.replace(_I18N_COMMENT_PREFIX_REGEXP, '').trim();
-          this._startSection(messages);
+          this._openTranslatableSection(comment, messages);
         }
       } else {
         if (isClosing) {
           if (this._depth == this._blockStartDepth) {
-            this._endSection(messages, this._blockChildren);
+            this._closeTranslatableSection(comment, messages, this._blockChildren);
             this._inI18nBlock = false;
             this._addMessage(messages, this._blockChildren, this._blockMeaningAndDesc);
           } else {
@@ -129,18 +125,15 @@ class _ExtractVisitor implements html.Visitor {
     this._mayBeAddBlockChildren(el);
     this._depth++;
     const wasInI18nNode = this._inI18nNode;
-    let useSection = false;
 
     // Extract only top level nodes with the (implicit) "i18n" attribute if not in a block or an ICU
     // message
     const i18nAttr = _getI18nAttr(el);
-    const isImplicitI18n =
+    const isImplicitI18n = this._implicitTags.some((tag: string): boolean => el.name === tag);
-        this._implicitTags.some((tagName: string): boolean => el.name === tagName);
     if (!(this._inI18nNode || this._inIcu || this._inI18nBlock)) {
       if (i18nAttr) {
         this._inI18nNode = true;
         this._addMessage(messages, el.children, i18nAttr.value);
-        useSection = true;
       } else if (isImplicitI18n) {
         this._inI18nNode = true;
         this._addMessage(messages, el.children);
@@ -155,10 +148,11 @@ class _ExtractVisitor implements html.Visitor {
 
     this._extractFromAttributes(el, messages);
 
-    if (useSection) {
+    if (i18nAttr || isImplicitI18n) {
-      this._startSection(messages);
+      // Start a section when the content is translatable
+      this._openTranslatableSection(el, messages);
       html.visitAll(this, el.children, messages);
-      this._endSection(messages, el.children);
+      this._closeTranslatableSection(el, messages, el.children);
     } else {
       html.visitAll(this, el.children, messages);
     }
@@ -171,6 +165,15 @@ class _ExtractVisitor implements html.Visitor {
     throw new Error('unreachable code');
   }
 
+  private _init(): void {
+    this._inI18nBlock = false;
+    this._inI18nNode = false;
+    this._depth = 0;
+    this._inIcu = false;
+    this._msgCountAtSectionStart = void 0;
+    this._errors = [];
+  }
+
   private _extractFromAttributes(el: html.Element, messages: Message[]): void {
     const explicitAttrNameToValue: {[k: string]: string} = {};
     const implicitAttrNames: string[] = this._implicitAttrs[el.name] || [];
@@ -214,20 +217,19 @@ class _ExtractVisitor implements html.Visitor {
   /**
    * Marks the start of a section, see `_endSection`
    */
-  private _startSection(messages: Message[]): void {
+  private _openTranslatableSection(node: html.Node, messages: Message[]): void {
-    if (this._sectionStartIndex !== void 0) {
+    if (this._msgCountAtSectionStart !== void 0) {
-      throw new Error('Unexpected section start');
+      this._reportError(node, 'Unexpected section start');
+    } else {
+      this._msgCountAtSectionStart = messages.length;
     }
-
-    this._sectionStartIndex = messages.length;
   }
 
   /**
    * Terminates a section.
    *
    * If a section has only one significant children (comments not significant) then we should not
-   * keep the message
+   * keep the message from this children:
-   * from this children:
    *
    * `<p i18n="meaning|description">{ICU message}</p>` would produce two messages:
    * - one for the <p> content with meaning and description,
@@ -239,12 +241,14 @@ class _ExtractVisitor implements html.Visitor {
    * Note that we should still keep messages extracted from attributes inside the section (ie in the
    * ICU message here)
    */
-  private _endSection(messages: Message[], directChildren: html.Node[]): void {
+  private _closeTranslatableSection(
-    if (this._sectionStartIndex === void 0) {
+      node: html.Node, messages: Message[], directChildren: html.Node[]): void {
-      throw new Error('Unexpected section end');
+    if (this._msgCountAtSectionStart === void 0) {
+      this._reportError(node, 'Unexpected section end');
+      return;
     }
 
-    const startIndex = this._sectionStartIndex;
+    const startIndex = this._msgCountAtSectionStart;
     const significantChildren: number = directChildren.reduce(
         (count: number, node: html.Node): number => count + (node instanceof html.Comment ? 0 : 1),
         0);
@@ -259,7 +263,7 @@ class _ExtractVisitor implements html.Visitor {
       }
     }
 
-    this._sectionStartIndex = void 0;
+    this._msgCountAtSectionStart = void 0;
   }
 
   private _reportError(node: html.Node, msg: string): void {

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

@@ -36,7 +36,7 @@ export class Icu implements Node {
   visit(visitor: Visitor, context?: any): any { return visitor.visitIcu(this, context); }
 }
 
-export class TagPlaceholder {
+export class TagPlaceholder implements Node {
   constructor(
       public tag: string, public attrs: {[k: string]: string}, public startName: string,
       public closeName: string, public children: Node[], public isVoid: boolean,
@@ -45,13 +45,13 @@ export class TagPlaceholder {
   visit(visitor: Visitor, context?: any): any { return visitor.visitTagPlaceholder(this, context); }
 }
 
-export class Placeholder {
+export class Placeholder implements Node {
   constructor(public value: string, public name: string = '', public sourceSpan: ParseSourceSpan) {}
 
   visit(visitor: Visitor, context?: any): any { return visitor.visitPlaceholder(this, context); }
 }
 
-export class IcuPlaceholder {
+export class IcuPlaceholder implements Node {
   constructor(public value: Icu, public name: string = '', public sourceSpan: ParseSourceSpan) {}
 
   visit(visitor: Visitor, context?: any): any { return visitor.visitIcuPlaceholder(this, context); }

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

@@ -13,10 +13,12 @@ import {getHtmlTagDefinition} from '../ml_parser/html_tags';
 import {InterpolationConfig} from '../ml_parser/interpolation_config';
 import {ParseSourceSpan} from '../parse_util';
 
-import {extractAstMessages} from './extractor';
+import {Message as HtmlMessage, extractAstMessages} from './extractor';
 import * as i18n from './i18n_ast';
 import {PlaceholderRegistry} from './serializers/placeholder';
 
+const _expParser = new ExpressionParser(new ExpressionLexer());
+
 /**
  * Extract all the i18n messages from a component template.
  */
@@ -29,11 +31,19 @@ export function extractI18nMessages(
     return [];
   }
 
-  const expParser = new ExpressionParser(new ExpressionLexer());
+  const htmlToI18n = getHtmlToI18nConverter(interpolationConfig);
-  const visitor = new _I18nVisitor(expParser, interpolationConfig);
 
-  return extractionResult.messages.map(
+  return extractionResult.messages.map(htmlToI18n);
-      (msg) => visitor.toI18nMessage(msg.nodes, msg.meaning, msg.description));
+}
+
+/**
+ * Returns a function converting html Messages to i18n Messages given an interpolationConfig
+ */
+export function getHtmlToI18nConverter(interpolationConfig: InterpolationConfig):
+    (msg: HtmlMessage) => i18n.Message {
+  const visitor = new _I18nVisitor(_expParser, interpolationConfig);
+
+  return (msg: HtmlMessage) => visitor.toI18nMessage(msg.nodes, msg.meaning, msg.description);
 }
 
 class _I18nVisitor implements html.Visitor {

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

@@ -37,14 +37,17 @@ export class MessageBundle {
         htmlParserResult.rootNodes, interpolationConfig, this._implicitTags, this._implicitAttrs);
 
     messages.forEach((message) => {
-      const id = strHash(serializeAst(message.nodes).join('') + `[${message.meaning}]`);
+      this._messageMap[messageDigest(message.nodes, message.meaning)] = message;
-      this._messageMap[id] = message;
     });
   }
 
   write(serializer: Serializer): string { return serializer.write(this._messageMap); }
 }
 
+export function messageDigest(nodes: i18n.Node[], meaning: string): string {
+  return strHash(serializeNodes(nodes).join('') + `[${meaning}]`);
+}
+
 /**
  * String hash function similar to java.lang.String.hashCode().
  * The hash code for a string is computed as
@@ -103,6 +106,6 @@ class _SerializerVisitor implements i18n.Visitor {
 
 const serializerVisitor = new _SerializerVisitor();
 
-export function serializeAst(ast: i18n.Node[]): string[] {
+export function serializeNodes(nodes: i18n.Node[]): string[] {
-  return ast.map(a => a.visit(serializerVisitor, null));
+  return nodes.map(a => a.visit(serializerVisitor, null));
 }

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

@@ -21,4 +21,8 @@ export class TranslationBundle {
       serializer: Serializer): TranslationBundle {
     return new TranslationBundle(serializer.load(content, 'url', placeholders));
   }
+
+  get(id: string): html.Node[] { return this._messageMap[id]; }
+
+  has(id: string): boolean { return id in this._messageMap; }
 }

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

@@ -10,7 +10,7 @@ import {ExtractionResult, extractAstMessages} from '@angular/compiler/src/i18n/e
 import {beforeEach, ddescribe, describe, expect, iit, inject, it, xdescribe, xit} from '@angular/core/testing/testing_internal';
 
 import {HtmlParser} from '../../src/ml_parser/html_parser';
-import {serializeAst} from '../html_parser/ast_serializer_spec';
+import {serializeNodes} from '../html_parser/ast_serializer_spec';
 
 export function main() {
   describe('MessageExtractor', () => {
@@ -21,6 +21,24 @@ export function main() {
         ]);
       });
 
+      it('should extract from elements', () => {
+        expect(
+            extract(
+                '<div i18n="m|d"><span i18n-title="m|d" title="single child">nested</span></div>'))
+            .toEqual([
+              [
+                ['<span i18n-title="m|d" title="single child">nested</span>'],
+                'm',
+                'd',
+              ],
+              [
+                ['title="single child"'],
+                'm',
+                'd',
+              ],
+            ]);
+      });
+
       it('should extract from elements', () => {
         expect(
             extract(
@@ -87,6 +105,11 @@ export function main() {
           [['{count, plural, =0 {text}}'], 'm', 'd'],
         ]);
 
+        // single message when ICU is the only (implicit) children
+        expect(extract('<div>{count, plural, =0 {text}}</div>', ['div'])).toEqual([
+          [['{count, plural, =0 {text}}'], '', ''],
+        ]);
+
         // one message for the element content and one message for the ICU
         expect(extract('<div i18n="m|d">before{count, plural, =0 {text}}after</div>')).toEqual([
           [['before', '{count, plural, =0 {text}}', 'after'], 'm', 'd'],
@@ -184,18 +207,24 @@ export function main() {
         it('should report nested translatable elements', () => {
           expect(extractErrors(`<p i18n><b i18n></b></p>`)).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b i18n>'],
+            ['Unexpected section start', '<b i18n>'],
+            ['Unexpected section end', '<p i18n>'],
           ]);
         });
 
         it('should report translatable elements in implicit elements', () => {
           expect(extractErrors(`<p><b i18n></b></p>`, ['p'])).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b i18n>'],
+            ['Unexpected section start', '<b i18n>'],
+            ['Unexpected section end', '<p>'],
           ]);
         });
 
         it('should report translatable elements in translatable blocks', () => {
           expect(extractErrors(`<!-- i18n --><b i18n></b><!-- /i18n -->`)).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b i18n>'],
+            ['Unexpected section start', '<b i18n>'],
+            ['Unexpected section end', '<!--'],
           ]);
         });
       });
@@ -246,18 +275,24 @@ export function main() {
         it('should report nested implicit elements', () => {
           expect(extractErrors(`<p><b></b></p>`, ['p', 'b'])).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b>'],
+            ['Unexpected section start', '<b>'],
+            ['Unexpected section end', '<p>'],
           ]);
         });
 
         it('should report implicit element in translatable element', () => {
           expect(extractErrors(`<p i18n><b></b></p>`, ['b'])).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b>'],
+            ['Unexpected section start', '<b>'],
+            ['Unexpected section end', '<p i18n>'],
           ]);
         });
 
         it('should report implicit element in translatable blocks', () => {
           expect(extractErrors(`<!-- i18n --><b></b><!-- /i18n -->`, ['b'])).toEqual([
             ['Could not mark an element as translatable inside a translatable section', '<b>'],
+            ['Unexpected section start', '<b>'],
+            ['Unexpected section end', '<!--'],
           ]);
         });
       });
@@ -285,7 +320,7 @@ function extract(
   // clang-format off
   // https://github.com/angular/clang-format/issues/35
   return messages.map(
-    message => [serializeAst(message.nodes), message.meaning, message.description, ]) as [string[], string, string][];
+    message => [serializeNodes(message.nodes), message.meaning, message.description, ]) as [string[], string, string][];
   // clang-format on
 }
 

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

@@ -8,9 +8,9 @@
 
 import {Message} from '@angular/compiler/src/i18n/i18n_ast';
 import {extractI18nMessages} from '@angular/compiler/src/i18n/i18n_parser';
-import {ddescribe, describe, expect, it} from '@angular/core/testing/testing_internal';
+import {ddescribe, describe, expect, iit, it} from '@angular/core/testing/testing_internal';
 
-import {serializeAst} from '../../src/i18n/message_bundle';
+import {serializeNodes} from '../../src/i18n/message_bundle';
 import {HtmlParser} from '../../src/ml_parser/html_parser';
 import {DEFAULT_INTERPOLATION_CONFIG} from '../../src/ml_parser/interpolation_config';
 
@@ -289,7 +289,7 @@ function _humanizeMessages(
   // clang-format off
   // https://github.com/angular/clang-format/issues/35
   return _extractMessages(html, implicitTags, implicitAttrs).map(
-    message => [serializeAst(message.nodes), message.meaning, message.description, ]) as [string[], string, string][];
+    message => [serializeNodes(message.nodes), message.meaning, message.description, ]) as [string[], string, string][];
   // clang-format on
 }
 

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

@@ -10,7 +10,7 @@ import * as i18n from '@angular/compiler/src/i18n/i18n_ast';
 import {Serializer} from '@angular/compiler/src/i18n/serializers/serializer';
 import {beforeEach, ddescribe, describe, expect, iit, inject, it, xdescribe, xit} from '@angular/core/testing/testing_internal';
 
-import {MessageBundle, serializeAst, strHash} from '../../src/i18n/message_bundle';
+import {MessageBundle, serializeNodes, strHash} from '../../src/i18n/message_bundle';
 import {HtmlParser} from '../../src/ml_parser/html_parser';
 import {DEFAULT_INTERPOLATION_CONFIG} from '../../src/ml_parser/interpolation_config';
 
@@ -59,7 +59,7 @@ export function main(): void {
 class _TestSerializer implements Serializer {
   write(messageMap: {[id: string]: i18n.Message}): string {
     return Object.keys(messageMap)
-        .map(id => `${id}=${serializeAst(messageMap[id].nodes)}`)
+        .map(id => `${id}=${serializeNodes(messageMap[id].nodes)}`)
         .join('//');
   }
 

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

@@ -12,7 +12,7 @@ import {beforeEach, ddescribe, describe, expect, iit, inject, it, xdescribe, xit
 
 import {HtmlParser} from '../../../src/ml_parser/html_parser';
 import {DEFAULT_INTERPOLATION_CONFIG} from '../../../src/ml_parser/interpolation_config';
-import {serializeAst} from '../../ml_parser/ast_serializer_spec';
+import {serializeNodes} from '../../ml_parser/ast_serializer_spec';
 
 export function main(): void {
   describe('XTB serializer', () => {
@@ -22,7 +22,7 @@ export function main(): void {
         {[id: string]: string} {
       const asAst = serializer.load(content, 'url', placeholders);
       let asText: {[id: string]: string} = {};
-      Object.keys(asAst).forEach(id => { asText[id] = serializeAst(asAst[id]).join(''); });
+      Object.keys(asAst).forEach(id => { asText[id] = serializeNodes(asAst[id]).join(''); });
 
       return asText;
     }

modules/@angular/compiler/test/ml_parser/ast_serializer_spec.ts

@@ -11,7 +11,7 @@ import * as html from '../../src/ml_parser/ast';
 import {HtmlParser} from '../../src/ml_parser/html_parser';
 
 export function main() {
-  describe('Node serilaizer', () => {
+  describe('Node serializer', () => {
     var parser: HtmlParser;
 
     beforeEach(() => { parser = new HtmlParser(); });
@@ -19,31 +19,31 @@ export function main() {
     it('should support element', () => {
       const html = '<p></p>';
       const ast = parser.parse(html, 'url');
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
 
     it('should support attributes', () => {
       const html = '<p k="value"></p>';
       const ast = parser.parse(html, 'url');
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
 
     it('should support text', () => {
       const html = 'some text';
       const ast = parser.parse(html, 'url');
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
 
     it('should support expansion', () => {
       const html = '{number, plural, =0 {none} =1 {one} other {many}}';
       const ast = parser.parse(html, 'url', true);
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
 
     it('should support comment', () => {
       const html = '<!--comment-->';
       const ast = parser.parse(html, 'url', true);
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
 
     it('should support nesting', () => {
@@ -55,7 +55,7 @@ export function main() {
         </p>                            
       </div>`;
       const ast = parser.parse(html, 'url', true);
-      expect(serializeAst(ast.rootNodes)).toEqual([html]);
+      expect(serializeNodes(ast.rootNodes)).toEqual([html]);
     });
   });
 }
@@ -91,6 +91,6 @@ class _SerializerVisitor implements html.Visitor {
 
 const serializerVisitor = new _SerializerVisitor();
 
-export function serializeAst(nodes: html.Node[]): string[] {
+export function serializeNodes(nodes: html.Node[]): string[] {
   return nodes.map(node => node.visit(serializerVisitor, null));
 }