fix(compiler): do not remove whitespace wrapping i18n expansions #31962
Conversation
petebacondarwin
added
type: bug/fix
action: review
petebacondarwin
force-pushed
the
i18n-expansion-ws-collapse
branch
from
fd25c66
to
4bfb845
Compare
petebacondarwin
force-pushed
the
i18n-expansion-ws-collapse
branch
from
4bfb845
to
3de571a
Compare
petebacondarwin
force-pushed
the
i18n-expansion-ws-collapse
branch
from
3de571a
to
5c85d80
Compare
There was a problem hiding this 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
added
state: blocked
and removed
action: review
|
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. |
|
I ran the following tests on
./scripts/build-packages-dist.sh
cd integration/cli-hello-world
yarn
--- 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
yarn ng serve
ng xi18n --i18n-format=xmb
ng xi18n --i18n-format=xlf2
mv src/messages.xlf src/messages.xlf2
ng xi18n --i18n-format=xlf
The generated files are completely identical including the generated IDs. This concurs with @alxhub's analysis of the code:
I think it is safe to say that this change will not break previously generated translations! |
petebacondarwin
force-pushed
the
i18n-expansion-ws-collapse
branch
from
5c85d80
to
227d136
Compare
petebacondarwin
added
action: merge
|
|
Thanks for additional checks @petebacondarwin! |
AndrewKushnir
added
the
action: presubmit
|
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. |
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
force-pushed
the
i18n-expansion-ws-collapse
branch
from
227d136
to
cf00fc0
Compare
|
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
added
risk: medium
merge: caretaker note
|
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. |
|
Thanks @AndrewKushnir 👍 |
|
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. |
|
The most recent Global TAP looks good (I removed the "blocked" label), we'll proceed with merging this PR soon. cc @kara |
) 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
…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
…ons (angular#31962)" This reverts commit 6ec91dd because it contains a breaking change (and thus should only be on the master branch).
…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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.
For example, the following template
was being collapsed to
which results in the text looking like
instead it should be collapsed to
which results in the text looking like
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information