[ivy] i18n extraction #22931

ocombe · 2018-03-22T16:41:36Z

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

Cannot run ng-xi18n with ivy

What is the new behavior?

Can use ng-xi18n with ivy

Does this PR introduce a breaking change?

[x] No

ocombe · 2018-03-22T16:42:27Z

packages/compiler-cli/src/transformers/program.ts

+        0) {
+      return {emitSkipped: true, diagnostics: [], emittedFiles: []};
+    }
+


I've moved this block of code here, because it's duplicated between both render2 and render3, but we can duplicate it if you prefer

mary-poppins · 2018-03-22T16:53:05Z

You can preview 6429758 at https://pr22931-6429758.ngbuilds.io/.

mary-poppins · 2018-03-22T16:53:46Z

You can preview 319d247 at https://pr22931-319d247.ngbuilds.io/.

mary-poppins · 2018-03-22T17:03:55Z

You can preview 7ebf68c at https://pr22931-7ebf68c.ngbuilds.io/.

mary-poppins · 2018-03-22T17:07:22Z

You can preview 5346322 at https://pr22931-5346322.ngbuilds.io/.

trotyl · 2018-03-23T09:28:17Z

packages/core/test/bundling/hello_world_i18n/index.ts

+// TODO(misko): Forgetting to export HelloWorld and not having NgModule fails silently.
+
+@NgModule({declarations: [HelloWorld]})
+export class INeedToExistEvenThoughtIAmNotNeeded {


EvenThought -> EvenThough?

ahah yeah probably, it's some code that misko wrote :)

vicb · 2018-03-28T16:04:10Z

packages/compiler-cli/test/extract_i18n_spec.ts

-  it('should not emit js', () => {
-    writeConfig();
-    writeSources();
+  describe('render3', () => {


move to a separate file

vicb · 2018-03-28T16:05:02Z

packages/compiler/src/template_parser/template_parser.ts

@@ -87,7 +87,7 @@ export class TemplateParser {
  constructor(
      private _config: CompilerConfig, private _reflector: CompileReflector,
      private _exprParser: Parser, private _schemaRegistry: ElementSchemaRegistry,
-      private _htmlParser: HtmlParser, private _console: Console,
+      public htmlParser: HtmlParser, private _console: Console,


mary-poppins · 2018-03-29T14:59:04Z

You can preview 0c403ab at https://pr22931-0c403ab.ngbuilds.io/.

mary-poppins · 2018-03-29T15:03:25Z

You can preview 894c5f4 at https://pr22931-894c5f4.ngbuilds.io/.

mary-poppins · 2018-03-29T15:09:43Z

You can preview 1e50c57 at https://pr22931-1e50c57.ngbuilds.io/.

ocombe · 2018-03-29T16:51:24Z

@chuckjaz I've moved the extraction from a separate visitor to TemplateDefinitionBuilder as @vicb requested, can you review that and tell me if it's ok, or if you have another idea in mind that would not make me change the definition of OutputContext? Thanks

vicb · 2018-04-26T23:29:01Z

packages/compiler/src/render3/r3_ast.ts

+  constructor(
+      public name: string, public value: string, public meta: I18nMeta,
+      public sourceSpan: ParseSourceSpan, public valueSpan?: ParseSourceSpan) {}
+  visit<Result>(visitor: I18nVisitor<Result>): Result { return visitor.visitI18nAttribute(this); }


should be visitI18nTextAttribute (I rename the visitITextAttribute in a pending PR)

I'll update once that PR gets merged

vicb · 2018-04-26T23:30:09Z

packages/compiler/src/render3/r3_ast.ts

@@ -70,6 +87,15 @@ export class Element implements Node {
  visit<Result>(visitor: Visitor<Result>): Result { return visitor.visitElement(this); }
 }

+export class I18nElement implements Node {


I'd like to see design for that. We probably do not need to duplicate all the props but wrap an Element

vicb · 2018-04-26T23:31:26Z

packages/compiler/src/render3/r3_ast.ts

    }
  } else {
    for (const node of nodes) {
      const newNode = node.visit(visitor);
      if (newNode) {
-        result.push(newNode);
+        Array.isArray(newNode) ? result.push(...newNode) : result.push(newNode);


hum let's see but should not be required

vicb · 2018-04-26T23:42:49Z

packages/compiler/src/render3/r3_template_transform.ts

@@ -46,6 +48,14 @@ const CLASS_ATTR = 'class';
 // Default selector used by `<ng-content>` if none specified
 const DEFAULT_CONTENT_SELECTOR = '*';

+/** Name of the i18n attributes **/


What we discussed about is:
html (string) -> htmlAST (nodes) -> ivyAST (nodes) -> i18IvyAST (nodes).

So this file (htmlAST (nodes) -> ivyAST (nodes)) must remain unchanged

And there should be a new transformer ivyAST (nodes) -> i18IvyAST (nodes).

Edit: thinking more about it, you probably can leave this here. You will need to add tests in r3_template_transform_spec - it's pending in a PR for now

waiting for that PR to be merged to add new tests

vicb · 2018-04-26T23:44:08Z

packages/compiler/src/render3/r3_view_compiler_local.ts

    }
+    const referenceDataSlots = new Map<string, number>();
+    const hasI18nAttr = element instanceof t.I18nElement;


NO !
We use the visitor pattern to avoid this kind of hacks.

vicb · 2018-04-26T23:45:58Z

packages/compiler/src/render3/r3_view_compiler_local.ts

-        outputAttrs[name] = attr;
-      }
+  visitElement(element: t.Element, elementIndex?: number) {
+    if (typeof elementIndex === 'undefined') {


NO!
Remove the extra params - is it not intended to pass extra argument when you like but it is part of the visitor pattern.

One way to refactor would be extract the common code in a common private function but the visitX methods should not be hacked. Make it hard to understand.

it was causing the tests to fail anyway

vicb · 2018-04-26T23:47:34Z

packages/compiler/src/render3/r3_view_compiler_local.ts

@@ -808,6 +751,10 @@ class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver {
        this.convertPropertyBinding(o.variable(CONTEXT_NAME), text.value));
  }

+  visitI18nBoundText(text: t.BoundText) {


Please remove until it is designed

The error message is explicit enough I think, it's the next step anyway, we might as well keep it

mary-poppins · 2018-04-30T10:11:17Z

You can preview a48e878 at https://pr22931-a48e878.ngbuilds.io/.

mary-poppins · 2018-05-07T13:07:37Z

You can preview c97330e at https://pr22931-c97330e.ngbuilds.io/.

mary-poppins · 2018-05-07T14:02:50Z

You can preview 60aa2b1 at https://pr22931-60aa2b1.ngbuilds.io/.

mary-poppins · 2018-05-07T15:52:37Z

You can preview 64fb4a6 at https://pr22931-64fb4a6.ngbuilds.io/.

ngbot · 2018-05-10T22:56:49Z

Hi @ocombe! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-05-10T22:58:54Z

Hi @ocombe! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mkljczk · 2018-06-08T03:03:51Z

Are you planning to resolve merge conflicts?

ocombe · 2018-06-08T08:13:46Z

No, I'm closing the PR, @vicb will take this over and recreate the PR with some refactoring.

angular-automatic-lock-bot · 2019-09-13T17:50:54Z

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: blocked action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n target: major This PR is targeted for the next major release labels Mar 22, 2018

ocombe assigned vicb Mar 22, 2018

ocombe requested a review from vicb March 22, 2018 16:41

googlebot added the cla: yes label Mar 22, 2018

ocombe commented Mar 22, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/ng-xi18n branch from 6429758 to 319d247 Compare March 22, 2018 16:43

ocombe added the comp: ivy label Mar 22, 2018

ocombe changed the title ~~Feat/ivy/ng xi18n~~ [ivy] i18n extraction Mar 22, 2018

ocombe force-pushed the feat/ivy/ng-xi18n branch from 319d247 to 7ebf68c Compare March 22, 2018 16:51

ocombe force-pushed the feat/ivy/ng-xi18n branch from 7ebf68c to 5346322 Compare March 22, 2018 16:53

trotyl reviewed Mar 26, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/ng-xi18n branch from 5346322 to 6a5957d Compare March 28, 2018 14:23

vicb reviewed Mar 28, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/ng-xi18n branch 3 times, most recently from 894c5f4 to 1e50c57 Compare March 29, 2018 14:58

ocombe force-pushed the feat/ivy/ng-xi18n branch from e372c30 to a00377f Compare March 29, 2018 16:48

ocombe requested a review from chuckjaz March 29, 2018 16:50

vicb reviewed Apr 26, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/ng-xi18n branch from d91252d to a48e878 Compare April 30, 2018 09:57

ocombe mentioned this pull request May 6, 2018

[i18n] plans #16477

Closed

20 tasks

ocombe added 3 commits May 7, 2018 14:36

feat(compiler-cli): i18n extraction with render3

9944f05

fixup! feat(compiler-cli): i18n extraction with render3

394376a

fixup! feat(compiler-cli): i18n extraction with render3

c97330e

ocombe force-pushed the feat/ivy/ng-xi18n branch from a48e878 to c97330e Compare May 7, 2018 12:52

fixup! feat(compiler-cli): i18n extraction with render3

64fb4a6

ocombe force-pushed the feat/ivy/ng-xi18n branch from 60aa2b1 to 64fb4a6 Compare May 7, 2018 15:40

ocombe added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 10, 2018

mhevery added the PR action: time-zone label May 30, 2018

ocombe closed this Jun 8, 2018

angular-automatic-lock-bot bot unassigned vicb Sep 13, 2019

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

[ivy] i18n extraction #22931

[ivy] i18n extraction #22931

ocombe commented Mar 22, 2018 •

edited

ocombe Mar 22, 2018

mary-poppins commented Mar 22, 2018

mary-poppins commented Mar 22, 2018

mary-poppins commented Mar 22, 2018

mary-poppins commented Mar 22, 2018

trotyl Mar 23, 2018

ocombe Mar 26, 2018

vicb Mar 28, 2018

vicb Mar 28, 2018

mary-poppins commented Mar 29, 2018

mary-poppins commented Mar 29, 2018

mary-poppins commented Mar 29, 2018

ocombe commented Mar 29, 2018

vicb Apr 26, 2018

ocombe Apr 30, 2018

vicb Apr 26, 2018

vicb Apr 26, 2018

vicb Apr 26, 2018 •

edited

ocombe May 7, 2018

vicb Apr 26, 2018 •

edited

vicb Apr 26, 2018 •

edited

ocombe Apr 30, 2018

vicb Apr 26, 2018 •

edited

ocombe Apr 30, 2018

mary-poppins commented Apr 30, 2018

mary-poppins commented May 7, 2018

mary-poppins commented May 7, 2018

mary-poppins commented May 7, 2018

ngbot bot commented May 10, 2018

ngbot bot commented May 10, 2018

mkljczk commented Jun 8, 2018

ocombe commented Jun 8, 2018

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

[ivy] i18n extraction #22931

[ivy] i18n extraction #22931

Conversation

ocombe commented Mar 22, 2018 • edited

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

mary-poppins commented Mar 22, 2018

mary-poppins commented Mar 22, 2018

mary-poppins commented Mar 22, 2018

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

mary-poppins commented Mar 29, 2018

mary-poppins commented Mar 29, 2018

mary-poppins commented Mar 29, 2018

ocombe commented Mar 29, 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

vicb Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Apr 26, 2018 • edited

Choose a reason for hiding this comment

vicb Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Apr 30, 2018

mary-poppins commented May 7, 2018

mary-poppins commented May 7, 2018

mary-poppins commented May 7, 2018

ngbot bot commented May 10, 2018

ngbot bot commented May 10, 2018

mkljczk commented Jun 8, 2018

ocombe commented Jun 8, 2018

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

ocombe commented Mar 22, 2018 •

edited

vicb Apr 26, 2018 •

edited

vicb Apr 26, 2018 •

edited

vicb Apr 26, 2018 •

edited

vicb Apr 26, 2018 •

edited