feat(ivy): runtime i18n #24037

ocombe · 2018-05-21T18:57:19Z

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

There is no runtime i18n for ivy

What is the new behavior?

Runtime i18n works for ivy
This is only the first step: html and expressions.
We still need to do ICUs, projected content, and the service for code translations.

Status

Done:

translate HTML elements (including moving/removing elements)
translate expressions (including moving/removing expressions)
translate attributes (including with expressions)
support multiple i18n blocks
support container instructions (with or without embedded template, including moving/removing them)
support ng-container
support projections (including with selectors, and multiple projections)
canonical tests for html/expressions/expressions in attributes for the compiler

To do:

updating design spec with recent changes

Does this PR introduce a breaking change?

[x] No

mary-poppins · 2018-05-21T21:11:46Z

You can preview 391ac36 at https://pr24037-391ac36.ngbuilds.io/.

mhevery · 2018-05-22T04:58:13Z

packages/core/src/render3/instructions.ts

+  EXPRESSION = 3 << 29,
+  CLOSE_NODE = 4 << 29,
+  REMOVE_NODE = 5 << 29,
+  UNMASK = (1 << 29) - 1,


rename to INDEX_MASK

mhevery · 2018-05-22T04:58:24Z

packages/core/src/render3/instructions.ts

+  CLOSE_NODE = 4 << 29,
+  REMOVE_NODE = 5 << 29,
+  UNMASK = (1 << 29) - 1,
+  MASK = ~((1 << 29) - 1),


Rename to INSTRUCTION_MASK

mhevery · 2018-05-22T04:58:55Z

packages/core/src/render3/instructions.ts

+  MASK = ~((1 << 29) - 1),
+}
+
+export function i18nMapping(msg: string, placeholders: string[]): any[] {


This will be public method needs detail public API comments.

mhevery · 2018-05-22T04:59:45Z

packages/core/src/render3/instructions.ts

+  const elementRegex = /\{\$(p_\d+)\}/g;
+  const expRegex = /\{\$(exp_\d+)\}/g;
+  const closeRegex = /\{\$(c_\w+)\}/g;
+  const tagRegex = /(\{\$\w+_\w+\})/g;


Every time this method runs it will allocate the above rgexps, which will create memory pressure. Move to top level.

mhevery · 2018-05-22T05:03:03Z

packages/core/src/render3/instructions.ts

+  MASK = ~((1 << 29) - 1),
+}
+
+export function i18nMapping(msg: string, placeholders: string[]): any[] {


can you type the return value better than any[] maybe I18nInstruction[] where type Instruction = number|string

mhevery · 2018-05-22T05:09:46Z

packages/core/src/render3/instructions.ts

+  // TODO(ocombe): find a way to cache this
+  const children = getChildren(previousOrParentNode);
+  for (let i = 0; i < children.length; i++) {
+    children.splice(i + 1, 0, ...getChildren(children[i]));


can you not use ... operators since when down-leveled to ES5 it is slow.
Notice that it causes apply to be called
http://www.typescriptlang.org/play/#src=children.splice(i%20%2B%201%2C%200%2C%20...getChildren(children%5Bi%5D))%3B

I can use slice & concat instead: https://jsperf.com/slice-concat-vs-splice-spread
It's 27% faster in chrome (5% slower in firefox).
Unless you have a better idea?

mhevery · 2018-05-22T05:13:06Z

packages/core/src/render3/instructions.ts

+  if (args.some(value => bind(value) !== NO_CHANGE)) {
+    const expRegex = /\{\$(exp_\d+)\}/g;
+    return msg.replace(
+        expRegex, (match: string, p1: string) => { return args[placeholders.indexOf(p1)]; });


You are allocating a regexp and than doing string replace. This will create GC pressure every time we run change detection and it is not the fastest. Can we refactor this so that it does not allocate anything when there is no change to inputs.

actually this is the same regex that it used above, I'll use the same constant

mhevery · 2018-05-22T05:13:23Z

packages/core/src/render3/instructions.ts

+  }
+}
+
+export function i18nInterpolation(


What is the point of this method with 7 args? Why 7?

I searched the answer that Victor did to me when I asked that and it was:

this is something we do in the runtime when we know the number of args, see interpolate1, interpolate2, ... interpolate8 and interpolateV. Creating an array consumes memory and has slower access so this is an optimization when we have only a few args (common case)

So that's probably not the implementation that he was suggesting, I'll switch to 8 functions instead

mhevery · 2018-05-22T05:14:35Z

packages/core/src/render3/instructions.ts

+/** A variable length version of i18nInterpolation */
+export function i18nInterpolationV(msg: string, placeholders: string[], args: any[]): string|
+    NO_CHANGE {
+  if (args.some(value => bind(value) !== NO_CHANGE)) {


Don't use some same argument as forEach

mhevery · 2018-05-22T05:16:01Z

packages/core/src/render3/node_manipulation.ts

@@ -456,6 +456,28 @@ export function appendChild(parent: LNode, child: RNode | null, currentView: LVi
  return false;
 }

+export function getChildren(parent: LNode): LNode[] {


The way you can make this method cacheable, is to collect method induces rather than LNodes themselves. So return would be number[]. Than you could do the dereference when processing instructions.

mary-poppins · 2018-05-22T16:27:12Z

You can preview 767144e at https://pr24037-767144e.ngbuilds.io/.

mary-poppins · 2018-05-22T16:51:56Z

You can preview b4a00f4 at https://pr24037-b4a00f4.ngbuilds.io/.

mhevery · 2018-05-22T18:27:09Z

packages/core/src/render3/instructions.ts

+    const instruction = instructions[i] as number;
+    switch (instruction & I18nFlags.INSTRUCTION_MASK) {
+      case I18nFlags.ELEMENT:
+        const element = children[instruction & I18nFlags.INDEX_MASK];


data[children[instruction & I18nFlags.INDEX_MASK]]

data[instruction & I18nFlags.INDEX_MASK]

mhevery · 2018-05-22T18:32:21Z

packages/core/src/render3/node_manipulation.ts

+  const children = [];
+  let currentNode = parent.child;
+  while (currentNode) {
+    if (currentNode.native) {


currentNode.tNode.index

JoostK · 2018-05-22T20:38:12Z

packages/core/src/render3/instructions.ts

+ */
+export function i18nInterpolation2(msg: string, ph0: string, ph1: string, a0: any, a1: any): string|
+    NO_CHANGE {
+  if (bind(a0) !== NO_CHANGE || bind(a1) !== NO_CHANGE) {


We could use bindingUpdatedN here, with N in {,2,4} to combine the bind checks. That would also get rid of the need for NO_CHANGE as then only booleans are handled. Refer to the interpolation# instructions where the same approach is used.

Applies to all i18nInterpolation# functions.

Also, watch out with short circuiting. We do not want that to happen, because it would cause bindings not to be updated. Recall that bind either updates or consumes a binding which causes bindingIndex to be incremented past the binding slot, we need to make sure that always occurs for the bindings to stay in sync.

Good points, I didn't think about that! thanks
and I should use stringify too

JoostK · 2018-05-22T20:39:32Z

packages/core/src/render3/instructions.ts

+    NO_CHANGE {
+  for (let i = 0; i < args.length; i++) {
+    if (bind(args[i]) !== NO_CHANGE) {
+      return msg.replace(


The early return here can't be right. We'd need to traverse all bindings for them to be updated and become string replaced.

JoostK · 2018-05-22T20:53:37Z

packages/core/src/render3/instructions.ts

+  for (let i = 0; i < msgList.length; i++) {
+    const value = msgList[i];
+    let match;
+    if (match = elementRegex.exec(value)) {


We can probably get away with a lot less regex matching here, if we were to change the regex into /\{\$((?:exp|p|c)_\w+)\}/g. We'd then know that all odd indices actually match, and we can easily figure out the instruction per the odd matches' prefix and the identifier from a substring. One regex match, then just string manipulation with known offsets.

Very good idea, thanks!

JoostK · 2018-05-24T10:44:59Z

packages/core/src/render3/instructions.ts

-    childrenIndex = previousOrParentNode.tNode !.childrenIndex !;
-  }
-
+  let children = data.slice(startIndex, endIndex + 1);


If we offset each lookup into data with startIndex, we'd void having to create a copy. We may then even get rid of endIndex, or use it only in dev mode for bounds checking.

mary-poppins · 2018-05-24T12:08:38Z

You can preview 1171f4c at https://pr24037-1171f4c.ngbuilds.io/.

mary-poppins · 2018-05-24T12:08:39Z

You can preview b528710 at https://pr24037-b528710.ngbuilds.io/.

vicb · 2018-05-24T17:59:14Z

packages/core/src/render3/instructions.ts

+
+/**
+ * A list of flags to encode the i18n instructions used to translate the template.
+ * We shift the flags by 29 so that 30 & 31 & 32 bits contains the instructions.


nit: use constant rather than number in the comment and in the enum to avoid then getting out of sync later

I tried that at first but for some reason typescript doesn't want a variable in a an enum value

vicb · 2018-05-24T18:00:52Z

packages/core/src/render3/instructions.ts

+  /** Used to decode the number encoded with the instruction. */
+  INDEX_MASK = (1 << 29) - 1,
+  /** Used to test the type of instruction. */
+  INSTRUCTION_MASK = ~((1 << 29) - 1),


nits:

0b111 << INSTRUCTION_SHIFT might be clearer / faster,

TEXT should read Text according to coding standards,

I would use Text = 1, Element = 2, ... InstructionShift = 29, probably clearer.

vicb · 2018-05-24T18:04:31Z

packages/core/src/render3/instructions.ts

+
+/** Represents the instructions used to translate the template. */
+export type I18nInstruction = number | string;
+const tagRegex = /\{\$((?:exp|p|c)_\w+)\}/g;


This is probably super slow and the reason why I had element and expression placeholder maps generated as distinct maps by the compiler.

Note: exp_..., p_..., c_... were only exemple names given in the i18n compiler spec, this will probably not be the final scheme

I don't see how to split the translation efficiently without a regex?

There should probably be a regexp, but a simpler/faster one ie /\{\$[^}]+\}/g
The other part of the perf gain is that placeholders would be specified as maps (one for element and an other one for expression).

vicb · 2018-05-24T18:05:18Z

packages/core/src/render3/instructions.ts

+ * Takes a translation string and the initial list of placeholders (elements and expressions)
+ * and returns a list of instructions that will be used to translate the template.
+ */
+export function i18nMapping(msg: string, placeholders: string[]): I18nInstruction[] {


nit: msg -> translation ?

vicb · 2018-05-24T18:05:34Z

packages/core/src/render3/instructions.ts

+ * and returns a list of instructions that will be used to translate the template.
+ */
+export function i18nMapping(msg: string, placeholders: string[]): I18nInstruction[] {
+  const instructions = [];


vicb · 2018-05-24T18:06:57Z

packages/core/src/render3/instructions.ts

+
+  for (let i = 0; i < msgList.length; i++) {
+    const value = msgList[i];
+    // odd index is either element, expression or close tag


// odd index are placeholders

vicb · 2018-05-24T18:07:18Z

packages/core/src/render3/instructions.ts

+  for (let i = 0; i < msgList.length; i++) {
+    const value = msgList[i];
+    // odd index is either element, expression or close tag
+    if ((i % 2) === 1) {


nit: if (i & 1) {...}

oh nice trick

vicb · 2018-05-24T18:08:38Z

packages/core/src/render3/instructions.ts

+    const value = msgList[i];
+    // odd index is either element, expression or close tag
+    if ((i % 2) === 1) {
+      switch (value.substr(0, 1)) {


value.charAt(0)

Note: p/e/c will change later

vicb · 2018-05-24T18:09:15Z

packages/core/src/render3/instructions.ts

+    if ((i % 2) === 1) {
+      switch (value.substr(0, 1)) {
+        case 'p':
+          instructions.push(placeholders.indexOf(value) | I18nFlags.ELEMENT);


why use .indexOf(value) instead of the map as proposed in the compiler side spec ?

good point, I've changed the code a few times, and now it seems like a map would be better, I'll update my code

ocombe · 2018-06-21T08:56:15Z

new fixup commit:

moved everything into i18n.ts
fixed reordering of exports for core_render3_private_export.ts
improved performance for i18nInterpolationV and added a test for it
moved detached property to tNode
other small stuff

mary-poppins · 2018-06-21T09:15:36Z

You can preview ffc36db at https://pr24037-ffc36db.ngbuilds.io/.

vicb · 2018-06-21T13:02:38Z

packages/core/src/render3/i18n.ts

+ * value will be concatenated into the final translation.
+ */
+export type I18nExpInstruction = number | string;
+/** Mapping of placeholder names to their absolute index in the current view. */


is this true for expression / embedded templates ?

yes, "in the current view" could probably be "in their templates" or something like that

vicb · 2018-06-21T13:03:15Z

packages/core/src/render3/i18n.ts

+
+/**
+ * Takes a translation string, the initial list of placeholders (elements and expressions) and the
+ * indexes of their corresponding expression nodes to return a list of instructions for each


indexes of their corresponding expression nodes unclear, as already commented

do you have something better in mind?

see the design spec.

vicb · 2018-06-21T13:03:40Z

packages/core/src/render3/i18n.ts

+/**
+ * Takes a translation string, the initial list of placeholders (elements and expressions) and the
+ * indexes of their corresponding expression nodes to return a list of instructions for each
+ * template.


template ->template function ?

vicb · 2018-06-21T13:16:32Z

packages/core/test/render3/compiler_canonical/i18n_spec.ts

+    }
+  });
+
+  it('should support expressions', () => {


add html to assert html vs expression indexes

there is already a test for that: "should support both html elements, expressions and expressions in attributes"

vicb · 2018-06-21T13:17:34Z

packages/core/test/render3/compiler_canonical/i18n_spec.ts

+    }
+  });
+
+  it('should support expressions in attributes', () => {


add a test for attributes without expression (unless there is one already ?)

missing a test for embedded templates

Attributes without expressions are static attributes, it's already tested in the compiler (nothing required at runtime).
There is a test for embedded templates line ~441

vicb · 2018-06-21T13:18:57Z

packages/core/test/render3/i18n_spec.ts

+    // Html tags are replaced by placeholders.
+    // Open tag placeholders are never re-used (closing tag placeholders can be).
+    const MSG_DIV_SECTION_1 = `{$p_3}trad 1{$c_p}{$p_0}trad 2{$p_1}trad 3{$c_p}{$c_p}`;
+    const i18n_1 = i18nMapping(MSG_DIV_SECTION_1, [{p_0: 1, p_1: 2, p_2: 3, p_3: 4}]);


change the placeholder name: if <tag> / </tag> -> START_TAG and END_TAG

vicb · 2018-06-21T13:20:02Z

packages/core/test/render3/i18n_spec.ts

+            '<div><a>trad expr 1</a>hello<b title="start expr 2 middle expr 1 end"><c>trad</c></b></div>');
+  });
+
+  describe('containers', () => {


view containers / embedded templates

vicb · 2018-06-21T13:21:59Z

packages/core/test/render3/i18n_spec.ts

+
+    it('should support sibling embedded templates', () => {
+      const MSG_DIV_SECTION_1 = `{$p_0}valeur: {$exp_1}!{$c_p}{$p_1}valeur bis: {$exp_2}!{$c_p}`;
+      // The indexes are based on each template function


There should be a canonical to assert that the compiler generates the correct code

vicb · 2018-06-21T13:23:15Z

packages/core/test/render3/i18n_spec.ts

+          // Initial template:
+          // <ul i18n>
+          //   <li *ngFor="let item of items">value: {{item}}</li>
+          // </ul>


for each test, please give:

the initial structure (the lines above),

the translation.

vicb · 2018-06-21T13:23:55Z

packages/core/test/render3/i18n_spec.ts

+              '<ul><li>valeur: one!</li><li>valeur: two!</li><li>valeur bis: one!</li><li>valeur bis: two!</li></ul>');
+    });
+
+    it('should support multiple levels of embedded templates', () => {


mhevery · 2018-06-21T14:40:05Z

http://test/OCL:201522516:BASE:201522609:1529591995239:2f0e49c4

ocombe · 2018-06-21T15:38:39Z

new fixup commit:

renamed all tags in the tests to START_... / END_...
added a canonical test for embedded templates

mary-poppins · 2018-06-21T15:47:39Z

You can preview fe62b7b at https://pr24037-fe62b7b.ngbuilds.io/.

mary-poppins · 2018-06-21T16:08:06Z

You can preview fad434f at https://pr24037-fad434f.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T18:22:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ocombe added state: WIP area: i18n target: major This PR is targeted for the next major release comp: ivy labels May 21, 2018

ocombe requested a review from mhevery May 21, 2018 18:57

googlebot added the cla: yes label May 21, 2018

This comment has been minimized.

Sign in to view

ocombe force-pushed the feat/ivy/runtime-i18n branch from 6cd2e22 to 391ac36 Compare May 21, 2018 20:47

mhevery suggested changes May 22, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/runtime-i18n branch from 767144e to b4a00f4 Compare May 22, 2018 16:39

mhevery reviewed May 22, 2018

View reviewed changes

JoostK requested changes May 22, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/runtime-i18n branch from b4a00f4 to 927a3cb Compare May 23, 2018 12:13

JoostK reviewed May 24, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/runtime-i18n branch 2 times, most recently from 1171f4c to b528710 Compare May 24, 2018 11:56

vicb reviewed May 24, 2018

View reviewed changes

fixup! feat(ivy): runtime i18n

d7dc836

ocombe force-pushed the feat/ivy/runtime-i18n branch from b697333 to 05d843d Compare June 21, 2018 08:53

fixup! feat(ivy): runtime i18n

ffc36db

ocombe force-pushed the feat/ivy/runtime-i18n branch from 05d843d to ffc36db Compare June 21, 2018 09:05

vicb reviewed Jun 21, 2018

View reviewed changes

mhevery approved these changes Jun 21, 2018

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 21, 2018

fixup! feat(ivy): runtime i18n

fad434f

ocombe force-pushed the feat/ivy/runtime-i18n branch from fe62b7b to fad434f Compare June 21, 2018 15:49

mhevery closed this in 84272e2 Jun 21, 2018

ocombe deleted the feat/ivy/runtime-i18n branch June 21, 2018 20:20

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

feat(ivy): runtime i18n #24037

feat(ivy): runtime i18n #24037

Conversation

ocombe commented May 21, 2018 • edited

PR Type

What is the current behavior?

What is the new behavior?

Status

Does this PR introduce a breaking change?

This comment has been minimized.

mary-poppins commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 22, 2018

mary-poppins commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK May 22, 2018 • edited

Choose a reason for hiding this comment

ocombe May 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 24, 2018

mary-poppins commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb May 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe commented Jun 21, 2018

mary-poppins commented Jun 21, 2018

Choose a reason for hiding this comment

ocombe Jun 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery commented Jun 21, 2018

ocombe commented Jun 21, 2018

mary-poppins commented Jun 21, 2018

mary-poppins commented Jun 21, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

ocombe commented May 21, 2018 •

edited

ocombe May 22, 2018 •

edited

JoostK May 22, 2018 •

edited

ocombe May 23, 2018 •

edited

vicb May 24, 2018 •

edited

ocombe Jun 21, 2018 •

edited