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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ng extract-i18n extracts incorrect equiv-text value #39671

Closed
kamilchlebek opened this issue Nov 13, 2020 · 7 comments
Closed

ng extract-i18n extracts incorrect equiv-text value #39671

kamilchlebek opened this issue Nov 13, 2020 · 7 comments
Labels
comp: i18n P3 Medium priority issue that needs to be resolved
Milestone

Comments

@kamilchlebek
Copy link

@kamilchlebek kamilchlebek commented Nov 13, 2020

馃悶 bug report

Affected Package

The issue is caused by package @angular/localize

Is this a regression?

Yes, the previous version in which this bug was not present was: v10

Description

Component template (it's usually formatted like this by prettier):

<div i18n>
  test {{ title }}
</div>

Running ng xi18n in angular v10, produced output that contained source element that looked:

<source>
   test <x id="INTERPOLATION" equiv-text="{{ title }}"/>
</source> 

Now after running ng extract-i18n it's:

<source> test <x id="INTERPOLATION" equiv-text="t {{ title "/>
</source>

馃敩 Minimal Reproduction

https://github.com/kamilchlebek/equiv-text-angular11 (see messages.xlf)

馃實 Your Environment

(it's a brand new project created using ng new)

Angular Version:

Angular CLI: 11.0.1
Node: 12.16.1
OS: linux x64

Angular: 11.0.0
... animations, common, compiler, compiler-cli, core, forms
... localize, platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1100.1
@angular-devkit/build-angular   0.1100.1
@angular-devkit/core            11.0.1
@angular-devkit/schematics      11.0.1
@angular/cli                    11.0.1
@schematics/angular             11.0.1
@schematics/update              0.1100.1
rxjs                            6.6.3
typescript                      4.0.5
@petebacondarwin petebacondarwin added comp: i18n P3 Medium priority issue that needs to be resolved labels Nov 13, 2020
@ngbot ngbot bot added this to the Backlog milestone Nov 13, 2020
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 13, 2020

I can confirm this bug. I am looking into it now.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 13, 2020

Just to give an update... these equiv-text properties are computed via the source-map from the generated code (e.g. app.component.js) back to the original source (e.g. app.component.html). In this case the source-map that was generated for this file is not accurate. See this visualization.

I think this is a bug in the Angular compiler, similar to one that I fixed recently. See #39486.

Investigating further...

@kamilchlebek
Copy link
Author

@kamilchlebek kamilchlebek commented Nov 13, 2020

That's bad 馃槥

As I workaround I could switch prettier htmlWhitespaceSensitivity to strict. Then the markup will look ugly:

<div i18n
  >test {{ title }}</div
>

but messages.xlf are okay.

Of course I'd rather avoid that for div and other elements being display:block by default.

https://prettier.io/blog/2018/11/07/1.15.0.html#whitespace-sensitive-formatting

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 14, 2020

I have found the cause of the problem. When we process the i18n tags, we run the processing twice. Once before whitespace has been removed (which results in correct source-spans) and then again after whitespace has been collapsed (which results in incorrect source-spans for placeholders).

See

if (i18nMetaVisitor.hasI18nMeta) {
rootNodes = html.visitAll(
new I18nMetaVisitor(interpolationConfig, /* keepI18nAttrs */ false), rootNodes);
}

A fix is in the works...

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Nov 16, 2020
When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes angular#39671
petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Nov 16, 2020
When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes angular#39671
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 16, 2020

Here's the fix: #39717. It was not trivial so it took a bit longer than I thought.

@kamilchlebek
Copy link
Author

@kamilchlebek kamilchlebek commented Nov 17, 2020

Great, thank you! Can't wait to see it released 馃殌

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Nov 19, 2020
When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes angular#39671
AndrewKushnir pushed a commit that referenced this issue Nov 20, 2020
鈥39717)

When the `preserveWhitespaces` is not true, the template parser will
process the parsed AST nodes to remove excess whitespace. Since the
generated `goog.getMsg()` statements rely upon the AST nodes after
this whitespace is removed, the i18n extraction must make a second pass.

Previously this resulted in innacurrate source-spans for the i18n text and
placeholder nodes that were extracted in the second pass.

This commit fixes this by reusing the source-spans from the first pass
when extracting the nodes in the second pass.

Fixes #39671

PR Close #39717
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 21, 2020

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
comp: i18n P3 Medium priority issue that needs to be resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants