Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): do not remove whitespace wrapping i18n expansions #31962

Conversation

@petebacondarwin
Copy link
Member

commented Aug 2, 2019

Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.

For example, the following template

<div>
  <strong>items left<strong> {count, plural, =1 {item} other {items}}
</div>

was being collapsed to

<div><strong>items left<strong>{count, plural, =1 {item} other {items}}</div>

which results in the text looking like

items left4

instead it should be collapsed to

<div><strong>items left<strong> {count, plural, =1 {item} other {items}}</div>

which results in the text looking like

items left 4

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@petebacondarwin petebacondarwin requested a review from angular/fw-compiler as a code owner Aug 2, 2019

@ngbot ngbot bot modified the milestone: needsTriage Aug 2, 2019

@googlebot googlebot added the cla: yes label Aug 2, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-expansion-ws-collapse branch from fd25c66 to 4bfb845 Aug 2, 2019

@petebacondarwin petebacondarwin requested a review from angular/fw-core as a code owner Aug 2, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-expansion-ws-collapse branch from 4bfb845 to 3de571a Aug 2, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-expansion-ws-collapse branch from 3de571a to 5c85d80 Aug 2, 2019

@AndrewKushnir
Copy link
Contributor

left a comment

Looks good to me, thanks @petebacondarwin 👍

One nit: the i18n_spec.ts file seems to be re-formatted, which is a bit unfortunate, since it's tough to see what's changed. I've seen similar issues when one of the strings in a test get too long (so I had to split it 'a' + 'b'). I'm wondering if we can find a long string that triggered the formatting (I believe you changed it as a part of this PR) and just split it, to keep the existing formatting.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I'm adding "blocked" label to this PR for now, since we need to verify whether these changes have an impact on extracted message ids and if so, we need to re-assess the approach. Thank you.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

I ran the following tests on integration/cli-hello-world in this branch vs master:

  • build packages and install files
./scripts/build-packages-dist.sh
cd integration/cli-hello-world
yarn
  • modify integration/cli-hello-world app as follows:
--- a/integration/cli-hello-world/src/app/app.component.html
+++ b/integration/cli-hello-world/src/app/app.component.html
@@ -5,7 +5,7 @@
 </div>
-<p>{{ 1 | percent }} awesome</p>
+<p i18n><strong>{{ awesomeness | percent }}</strong> {awesomeness, plural, =0 {rubbish} =1 {amazing} other {unbelievable}}</p>
 <h2>Here are some links to help you start: </h2>
 <ul>
   <li>
--- a/integration/cli-hello-world/src/app/app.component.ts
+++ b/integration/cli-hello-world/src/app/app.component.ts
@@ -7,4 +7,5 @@ import { Component } from '@angular/core';
 })
 export class AppComponent {
   title = 'app';
+  awesomeness = 2;
 }

Note the space between ...</strong> and {awesomeness, ...

  • ensure the app works as expected (display space in this branch - no space in `master):
yarn ng serve
  • extract the i18n messages:
ng xi18n --i18n-format=xmb
ng xi18n --i18n-format=xlf2
mv src/messages.xlf src/messages.xlf2
ng xi18n --i18n-format=xlf
  • compare the extracted i18n files for this branch and master:

The generated files are completely identical including the generated IDs.


This concurs with @alxhub's analysis of the code:

The logic to strip whitespaces is here: https://github.com/angular/angular/blob/master/packages/compiler/src/render3/view/template.ts#L1935-L1951
Basically, we take i18n IDs before any whitespace stripping, then re-process i18n afterwards to deal with any changes resulting from whitespace changes.
But we save the IDs from before. So i18n IDs are in theory not affected by whitespace changes?


I think it is safe to say that this change will not break previously generated translations!

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-expansion-ws-collapse branch from 5c85d80 to 227d136 Aug 3, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

@AndrewKushnir :

  • No breaking change demonstrated...
  • Cleaned up re-formatted spec
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

Thanks for additional checks @petebacondarwin!

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

It looks like this change breaks a couple targets in our Blueprint g3 presubmit. We'll need to perform further investigation and run Global TAP to see the total number of failed targets. I'll mark this PR as "blocked" for now. Thank you.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

fix(compiler): do not remove whitespace wrapping i18n expansions
Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.

For example, the following template

```html
<div>
  <strong>items left<strong> {count, plural, =1 {item} other {items}}
</div>
```

was being collapsed to

```html
<div><strong>items left<strong>{count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left4
```

instead it should be collapsed to

```html
<div><strong>items left<strong> {count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left 4
```

---

**Analysis of the code and manual testing has shown that this does not cause
the generated ids to change, so there is no breaking change here.**

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-expansion-ws-collapse branch from 227d136 to cf00fc0 Aug 4, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

g3 targets were updated to be forward-compatible with this change. I started a new Global TAP Presubmit to verify there are no additional targets that might be affected. Thank you.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Note to Caretaker: Global TAP is currently "green". However there might still be a risk of having broken targets, so it'd be great to make a separate g3 sync for these changes (i.e. do not bundle them with other changes in g3 sync). Thank you.

@alxhub

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Thanks @AndrewKushnir 👍

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

FYI, merge of this PR was re-scheduled to tomorrow morning (PST time) due to release. I've started a new Global TAP to make sure we do not have new targets affected by this change.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

The most recent Global TAP looks good (I removed the "blocked" label), we'll proceed with merging this PR soon.

cc @kara

kara added a commit that referenced this pull request Aug 9, 2019

fix(compiler): do not remove whitespace wrapping i18n expansions (#31962
)

Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.

For example, the following template

```html
<div>
  <strong>items left<strong> {count, plural, =1 {item} other {items}}
</div>
```

was being collapsed to

```html
<div><strong>items left<strong>{count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left4
```

instead it should be collapsed to

```html
<div><strong>items left<strong> {count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left 4
```

---

**Analysis of the code and manual testing has shown that this does not cause
the generated ids to change, so there is no breaking change here.**

PR Close #31962

@kara kara closed this in 0ddf0c4 Aug 9, 2019

@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-expansion-ws-collapse branch Aug 11, 2019

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019

fix(compiler): do not remove whitespace wrapping i18n expansions (ang…
…ular#31962)

Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.

For example, the following template

```html
<div>
  <strong>items left<strong> {count, plural, =1 {item} other {items}}
</div>
```

was being collapsed to

```html
<div><strong>items left<strong>{count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left4
```

instead it should be collapsed to

```html
<div><strong>items left<strong> {count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left 4
```

---

**Analysis of the code and manual testing has shown that this does not cause
the generated ids to change, so there is no breaking change here.**

PR Close angular#31962

kara added a commit to kara/angular that referenced this pull request Aug 12, 2019

Revert "fix(compiler): do not remove whitespace wrapping i18n expansi…
…ons (angular#31962)"

This reverts commit 6ec91dd because it contains
a breaking change (and thus should only be on the master branch).

kara added a commit that referenced this pull request Aug 12, 2019

Revert "fix(compiler): do not remove whitespace wrapping i18n expansi…
…ons (#31962)" (#32110)

This reverts commit 6ec91dd because it contains
a breaking change (and thus should only be on the master branch).

PR Close #32110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.