feat(compiler): add "original" placeholder value on extracted XMB #25079

Goodwine · 2018-07-24T23:55:33Z

Update XMB placeholders(<ph>) to include the original value on top of an
example. Placeholders can by definition have one example(<ex>) tag and a
text node. The text node is used by TC as the "original" value from the
placeholder, while the example should represent a dummy value.
For example: <ph name="PET"><ex>Gopher</ex>{{ petName }}</ph>.
This change makes sure that we have the original text, but it DOES NOT
make sure that the example is correct. The example has the same wrong
behavior of showing the interpolation text rather than a useful example.

No breaking changes, but tools that depend on the previous behavior and
don't consider the full XMB definition may fail to parse the XMB.
Fixes b/72565847

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[x] Other... Please describe: XMB extraction

What is the current behavior?

A message like <foo>Name: {{yourName}}</foo> would generate this xmb message:

<msg id=123>Name: <ph name="INTERPOLATION"><ex>{{yourName}}</ex></ph></msg>

Issue Number: http://b/72565847

What is the new behavior?

A message like <foo>Name: {{yourName}}</foo> would generate this xmb message:

<msg id=123>Name: <ph name="INTERPOLATION"><ex>{{yourName}}</ex>{{yourName}}</ph></msg>

The <ph> PCDATA (text node) is used by TC for some validations explained further in the internal bug.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Tools that depend on the current (wrong) implementation may fail if they don't take the #PCDATA definition into account.

Other information

Ideally the example should be something like <ex>Carlos</ex>, but fixing the example is out of scope because there is no way to provide one.

Goodwine · 2018-07-24T23:57:40Z

Note: These bazel tests were already failing

//packages/compiler-cli/test/compliance:compliance
//packages/core/test/bundling/hello_world:symbol_test
//packages/core/test/bundling/hello_world:test
//packages/core/test/bundling/todo:symbol_test
//packages/core/test/bundling/todo:test

vicb · 2018-07-25T00:10:23Z

packages/compiler-cli/test/BUILD.bazel

@@ -43,6 +43,7 @@ jasmine_node_test(
    ],
    deps = [
        ":extract_i18n_lib",
+        "//packages/common:npm_package",


Could you explain why this change is required ? Thx

CommonModule is needed to process ICU syntax, otherwise angular complains that it doesn't know how to handle ICU. The module is imported in a generated "module" test file similar to @NgModule, but CommonModule is in a different package.

Why not importing //packages/common instead ?

I copied it from another rule, when changing it to //packages/common, bazel fails the test //packages/compiler-cli/test:extract_i18n because "error TS2307: Cannot find module '@angular/common'."

vicb · 2018-07-25T00:10:59Z

@Goodwine Thanks for the PR. I'll do my best to review it by Thursday.

vicb · 2018-07-25T01:00:36Z

packages/compiler/src/i18n/serializers/xmb.ts

@@ -115,30 +115,36 @@ class _Visitor implements i18n.Visitor {
  }

  visitTagPlaceholder(ph: i18n.TagPlaceholder, context?: any): xml.Node[] {
-    const startEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(`<${ph.tag}>`)]);
-    const startTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.startName}, [startEx]);
+    const originalStartTag = new xml.Text(`<${ph.tag}>`);


nit: rename to startTagAsText or something along those line

Please also add a comment explaining that the TC uses this extra text node (for all placeholder types).

vicb · 2018-07-25T01:01:10Z

packages/compiler/src/i18n/serializers/xmb.ts

    if (ph.isVoid) {
      // void tags have no children nor closing tags
      return [startTagPh];
    }

-    const closeEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(`</${ph.tag}>`)]);
-    const closeTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.closeName}, [closeEx]);
+    const originalCloseTag = new xml.Text(`</${ph.tag}>`);


nit: closeTagAsText

vicb · 2018-07-25T01:03:30Z

packages/compiler/src/i18n/serializers/xmb.ts

-    ]);
-    return [new xml.Tag(_PLACEHOLDER_TAG, {name: ph.name}, [exTag])];
+    const originalIcu = new xml.Text(
+        `{${ph.value.expression}, ${ph.value.type}, ${Object.keys(ph.value.cases).map((value: string) => value + ' {...}').join(' ')}}`);


nit: would you mind extracting the ${} to local consts ?
(I know this has not been introduced by you but it would help readability anyway)

vicb · 2018-07-25T01:10:15Z

packages/compiler/test/i18n/integration_xmb_xtb_spec.ts

  <msg id="703464324060964421"><source>file.ts:22,24</source>
-        <ph name="ICU"><ex>{sex, select, male {...} female {...} other {...}}</ex></ph>
+        <ph name="ICU"><ex>male, female, other</ex>{sex, select, male {...} female {...} other {...}}</ph>


is changing the <ex> a requirement from the TC

it's not, <ex> is used by translators to know what kind of values to expect, I figured it would make sense to only list the options instead of the icu's text representation

I would prefer not to change this one in this PR, Coud you please revert this change.

vicb · 2018-07-25T01:13:02Z

packages/compiler/test/i18n/integration_xmb_xtb_spec.ts

-  <msg id="3780349238193953556"><source>file.ts:9</source><source>file.ts:10</source><ph name="START_ITALIC_TEXT"><ex>&lt;i&gt;</ex></ph>with placeholders<ph name="CLOSE_ITALIC_TEXT"><ex>&lt;/i&gt;</ex></ph></msg>
-  <msg id="5415448997399451992"><source>file.ts:11</source><ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex></ph>with <ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex></ph>nested<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex></ph> placeholders<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex></ph></msg>
+  <msg id="3780349238193953556"><source>file.ts:9</source><source>file.ts:10</source><ph name="START_ITALIC_TEXT"><ex>&lt;i&gt;</ex>&lt;i&gt;</ph>with placeholders<ph name="CLOSE_ITALIC_TEXT"><ex>&lt;/i&gt;</ex>&lt;/i&gt;</ph></msg>
+  <msg id="5415448997399451992"><source>file.ts:11</source><ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex>&lt;div&gt;</ph>with <ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex>&lt;div&gt;</ph>nested<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex>&lt;/div&gt;</ph> placeholders<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex>&lt;/div&gt;</ph></msg>


<div> looks wrong here - is it github transforming encoded entities ?

This is the XML library when doing new xml.Text(`<${ph.tag}>`);, it's WAI

jermowery · 2018-07-25T01:21:50Z

+100,000,000 this will eliminate an enormous annoyance

vicb

Thank you very much for this PR.
Could you please address the minor comments ?

Goodwine · 2018-07-25T20:48:30Z

Thanks for the review, I've addressed the comments, PTAL

vicb · 2018-07-25T21:39:53Z

Could you please revert the <ex> change - everything else is great.
Thanks !

edit:

CircleCi issues should be solved after a rebase,
I have restarted Travis - very flaky ATM

Update XMB placeholders(<ph>) to include the original value on top of an example. Placeholders can by definition have one example(<ex>) tag and a text node. The text node is used by TC as the "original" value from the placeholder, while the example should represent a dummy value. For example: <ph name="PET"><ex>Gopher</ex>{{ petName }}</ph>. This change makes sure that we have the original text, but it *DOES NOT* make sure that the example is correct. The example has the same wrong behavior of showing the interpolation text rather than a useful example. No breaking changes, but tools that depend on the previous behavior and don't consider the full XMB definition may fail to parse the XMB. Fixes b/72565847

Goodwine · 2018-07-25T23:25:20Z

PTAL
Alright, I reverted the ICU <ex> change. I also rebased to upstream/master so hopefully circleci won't fail now. The 5 test targets I mentioned earlier are still failing tho.

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

http://test/OCL:206611829:BASE:206611877:1532973875793:3559563b

vicb · 2018-07-30T18:51:28Z

merge-assistance: missing pullapprove but the change is only about a symbol that has been renamed (to fix a typo). Please merge. Thanks.

…gular#25079) Update XMB placeholders(<ph>) to include the original value on top of an example. Placeholders can by definition have one example(<ex>) tag and a text node. The text node is used by TC as the "original" value from the placeholder, while the example should represent a dummy value. For example: <ph name="PET"><ex>Gopher</ex>{{ petName }}</ph>. This change makes sure that we have the original text, but it *DOES NOT* make sure that the example is correct. The example has the same wrong behavior of showing the interpolation text rather than a useful example. No breaking changes, but tools that depend on the previous behavior and don't consider the full XMB definition may fail to parse the XMB. Fixes b/72565847 PR Close angular#25079

angular-automatic-lock-bot · 2019-09-14T22:23:37Z

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.}

googlebot added the cla: yes label Jul 24, 2018

vicb added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 25, 2018

vicb reviewed Jul 25, 2018

View reviewed changes

vicb approved these changes Jul 25, 2018

View reviewed changes

vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 25, 2018

vicb added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 30, 2018

vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 30, 2018

IgorMinar approved these changes Jul 30, 2018

View reviewed changes

IgorMinar removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 30, 2018

IgorMinar closed this in e99d860 Jul 30, 2018

elvisbegovic mentioned this pull request Oct 19, 2018

v7.0.0 bugs and 4/5x slower serve & compile AOT angular/angular-cli#12646

Closed

Goodwine deleted the ex-to-orig-squashed branch April 4, 2019 22:37

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): add "original" placeholder value on extracted XMB #25079

feat(compiler): add "original" placeholder value on extracted XMB #25079

Goodwine commented Jul 24, 2018 •

edited

Goodwine commented Jul 24, 2018

vicb Jul 25, 2018

Goodwine Jul 25, 2018

vicb Jul 25, 2018

Goodwine Jul 25, 2018

vicb commented Jul 25, 2018

vicb Jul 25, 2018

vicb Jul 25, 2018

vicb Jul 25, 2018

vicb Jul 25, 2018

Goodwine Jul 25, 2018

vicb Jul 25, 2018

Goodwine Jul 25, 2018

vicb Jul 25, 2018

vicb Jul 25, 2018

Goodwine Jul 25, 2018

jermowery commented Jul 25, 2018

vicb left a comment

Goodwine commented Jul 25, 2018

vicb commented Jul 25, 2018 •

edited

Goodwine commented Jul 25, 2018

vicb commented Jul 30, 2018

vicb commented Jul 30, 2018

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

feat(compiler): add "original" placeholder value on extracted XMB #25079

feat(compiler): add "original" placeholder value on extracted XMB #25079

Conversation

Goodwine commented Jul 24, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Goodwine commented Jul 24, 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 commented Jul 25, 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

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

jermowery commented Jul 25, 2018

vicb left a comment

Choose a reason for hiding this comment

Goodwine commented Jul 25, 2018

vicb commented Jul 25, 2018 • edited

Goodwine commented Jul 25, 2018

vicb commented Jul 30, 2018

vicb commented Jul 30, 2018

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

Goodwine commented Jul 24, 2018 •

edited

vicb commented Jul 25, 2018 •

edited