Building blocks for ivy/i18n + demo #22654

ocombe · 2018-03-08T15:54:10Z

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the new behavior?

Runtime i18n

Does this PR introduce a breaking change?

[x] No

vicb · 2018-03-08T19:34:19Z

packages/compiler/src/render3/r3_view_compiler.ts

 import {Identifiers as R3} from './r3_identifiers';
+import {_I18N_ATTR, _I18N_ATTR_PREFIX} from "../i18n/extractor_merger";


duplicate ? (extractor_merger will be removed)

vicb · 2018-03-08T19:39:03Z

packages/compiler/test/render3/r3_view_compiler_spec.ts

+      };
+
+      const template = `
+          import * as $g1$ from 'goog';


Seems like we should not import (assume it is available globally)

vicb · 2018-03-08T19:40:32Z

packages/compiler/test/render3/r3_view_compiler_spec.ts

+      const template = `
+          import * as $g1$ from 'goog';
+          …
+          const $c1$ = ['title', $g1$.getMsg('introduction')];


and generate goog.getMsg(...) enforce "goog".
Also I think we should generate:

// There might be a comment here to add description and meaning const UPPER_CASE_NAME = goog.getMsg('...'); const $c1$ = ['title', UPPER_CASE_NAME];

vicb · 2018-03-08T19:41:46Z

packages/compiler/test/render3/r3_view_compiler_spec.ts

+                @Component({
+                  selector: 'my-component',
+                  template: \`
+                    <div i18n i18n-title title="introduction">Hello <b>{{name}}<i>!</i><i>!</i></b></div>


Update to only translate:

static text,

static attributes.

For this first version we will not handle html nor expression nor ICU

vicb · 2018-03-08T19:43:45Z

packages/compiler/test/render3/r3_view_compiler_spec.ts

+          const $c1$ = ['title', $g1$.getMsg('introduction')];
+          …
+          template: function MyComponent_Template(ctx: IDENT, cm: IDENT) {
+            const $r1$ = {b:[2], i:[4, 6]};


placeholder names should be quoted (else they might get renamed by Closure)

vicb · 2018-03-08T19:46:54Z

I have added some inline comments.

As a general rule please add commits with finer granularity:

refactor(ivy): removed unused exports,
fix(ivy): ...,

It is easier to start with multiple commits and to squash them if there are too many rather than split commits afterwards.

Having multiple commits ease the reviewer job - and we will probably have multiple rounds of review here. Thanks.

mary-poppins · 2018-03-09T16:57:07Z

You can preview d790f3f at https://pr22654-d790f3f.ngbuilds.io/.

vicb · 2018-03-12T21:56:51Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -327,6 +338,11 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
  private _valueConverter: ValueConverter;
  private unsupported = unsupported;
  private invalid = invalid;
+  private _phMaps: {[name: string]: number[]}[] = [{}];


_phMaps vs _placeholderRegistry be consistent with naming please.
A comment on phMaps (and other props) would certainly be helpful

I'll remove placeholderRegistry since I don't need it anymore (it's a leftover from previous code).
None of the other props has comments on it, that's why I didn't add comments on those either (you usually tell me to remove the comments that you find useless)

remember what you told me this morning about the lack of comments ;)

vicb · 2018-03-12T21:57:42Z

packages/compiler/src/render3/r3_view_compiler.ts

    const elementIndex = this.allocateDataSlot();
    let componentIndex: number|undefined = undefined;
    const referenceDataSlots = new Map<string, number>();
+    let astAttrs = ast.attrs;
+
+    /** Start i18n **/


"Start i18n" ?

Just a small placeholder to help me find my code for now :D
I'll remove it since I found out about bookmarks on webstorm!

vicb · 2018-03-12T21:58:11Z

packages/compiler/src/render3/r3_view_compiler.ts

+        this._phMapIndex += 1;
+        this._phMaps[this._phMapIndex] = {};
+        this._placeholderRegistry = new PlaceholderRegistry();
+      }


vicb · 2018-03-12T22:00:12Z

packages/compiler/src/render3/r3_view_compiler.ts

+      if(attr.name === I18N_ATTR) {
+        this._inI18n = true;
+        this._i18nBlockMeta = this.parseI18nMeta(attr.value);
+        inI18nNode = true;


vs in18n ?
Maybe inI18nNode vs isI18nNode would be clearer ?

vicb · 2018-03-12T22:01:42Z

packages/compiler/src/render3/r3_view_compiler.ts

+
+    if(this._inI18n) {
+      if(!inI18nNode) {
+        const phName = ast.name.toLowerCase();


ast.name.toLowerCase() could lead to collisions.
How are those handled ?

the same way they are handled in src/i18n/serializers/placeholder.ts right now: they're not
a tag name is the same thing in lowercase and in uppercase (ie <DIV> and <div> are equivalent), I don't think that there is a risk of collision here, unless there's a special case I'm not aware of?

tag names are case sensitive in angular.

ocombe · 2018-03-22T12:49:51Z

removing the merge label until we can make the CircleCI build pass

mary-poppins · 2018-03-22T19:37:02Z

You can preview a29a3ed at https://pr22654-a29a3ed.ngbuilds.io/.

PR Close #22654

…gular#22654) PR Close angular#22654

PR Close angular#22654

ronnykristiansen · 2018-04-24T14:07:18Z

Runtime i18n, is this now supported i rc5, is there an example on how this works with properties files etc not only the testcase?

ocombe · 2018-04-24T14:27:54Z

You can set enableIvy to true in the angularCompilerOptions of your tsconfig.json to make it work, but for now it's only static string translations (for text and attributes).
Otherwise it works exactly the same as before, with the i18n attribute on the elements that you want to translate.
The extraction doesn't work with enableIvy (the PR hasn't been merged yet), but you can set it to false, extract, and set it to true again, the old translations files should work for ivy (the only difference in generated translation files right now is the info about the location in the template, but it's only optional stuff for translation tools).

angular-automatic-lock-bot · 2019-09-13T13:51:41Z

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 Mar 8, 2018

ocombe requested review from vicb and chuckjaz March 8, 2018 15:54

googlebot added the cla: yes label Mar 8, 2018

vicb reviewed Mar 8, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/generate-i18n-template-code branch 4 times, most recently from 2361137 to d3fdd3b Compare March 9, 2018 16:23

angular deleted a comment from mary-poppins Mar 9, 2018

angular deleted a comment from ngbot bot Mar 9, 2018

vicb reviewed Mar 12, 2018

View reviewed changes

angular deleted a comment from mary-poppins Mar 22, 2018

ocombe removed the action: merge The PR is ready for merge by the caretaker label Mar 22, 2018

ocombe mentioned this pull request Mar 22, 2018

[ivy] i18n extraction #22931

Closed

vicb mentioned this pull request Mar 22, 2018

Disable ng_package test temporarily #22933

Closed

ocombe added 2 commits March 22, 2018 12:28

feat(compiler): add basic support for in ivy/i18n code generation

2d40dc6

test(core): hello world i18n demo

a29a3ed

vicb force-pushed the feat/ivy/generate-i18n-template-code branch from 3d92a9f to a29a3ed Compare March 22, 2018 19:28

vicb added the action: merge The PR is ready for merge by the caretaker label Mar 22, 2018

vicb mentioned this pull request Mar 22, 2018

refactor(compiler): move handling of translations to the ConstantPool #22942

Closed

matsko closed this in 204ba9d Mar 22, 2018

matsko pushed a commit that referenced this pull request Mar 22, 2018

test(core): hello world i18n demo (#22654)

99711b1

PR Close #22654

ocombe deleted the feat/ivy/generate-i18n-template-code branch March 23, 2018 13:09

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(compiler): add basic support for in ivy/i18n code generation (an…

9405d84

…gular#22654) PR Close angular#22654

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

test(core): hello world i18n demo (angular#22654)

5c20303

PR Close angular#22654

vicb mentioned this pull request Mar 26, 2018

test(ivy): create todo app in ivy #22921

Closed

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Building blocks for ivy/i18n + demo #22654

Building blocks for ivy/i18n + demo #22654

ocombe commented Mar 8, 2018

vicb Mar 8, 2018

vicb Mar 8, 2018

vicb Mar 8, 2018 •

edited

vicb Mar 8, 2018

vicb Mar 8, 2018

vicb commented Mar 8, 2018

mary-poppins commented Mar 9, 2018

vicb Mar 12, 2018

ocombe Mar 13, 2018

vicb Mar 13, 2018

vicb Mar 12, 2018

ocombe Mar 13, 2018 •

edited

vicb Mar 12, 2018

vicb Mar 12, 2018

vicb Mar 12, 2018

ocombe Mar 13, 2018

vicb Mar 13, 2018

ocombe commented Mar 22, 2018 •

edited

mary-poppins commented Mar 22, 2018

ronnykristiansen commented Apr 24, 2018

ocombe commented Apr 24, 2018

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

		import {Identifiers as R3} from './r3_identifiers';
		import {_I18N_ATTR, _I18N_ATTR_PREFIX} from "../i18n/extractor_merger";

Building blocks for ivy/i18n + demo #22654

Building blocks for ivy/i18n + demo #22654

Conversation

ocombe commented Mar 8, 2018

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Mar 8, 2018

mary-poppins commented Mar 9, 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

ocombe Mar 13, 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

ocombe commented Mar 22, 2018 • edited

mary-poppins commented Mar 22, 2018

ronnykristiansen commented Apr 24, 2018

ocombe commented Apr 24, 2018

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

vicb Mar 8, 2018 •

edited

ocombe Mar 13, 2018 •

edited

ocombe commented Mar 22, 2018 •

edited