fix(compiler): fix i18n IDs generation, add a migration tool by ocombe · Pull Request #17638 · angular/angular

ocombe · 2017-06-20T16:54:46Z

Please check if the PR fulfills these requirements

Tests for the changes have been added

What kind of change does this PR introduce?

Bugfix

What is the current behavior?
See #15573.

What is the new behavior?
When we extract messages we digest them to generate ids (unless the id is provided).
With this fix we ignore the content of the placeholder when we digest to only use the fact that it's a placeholder and its position.
We already did the same thing with ICU expressions for XMB and this previous fix is now also applied to ICU expressions for XLIFF (now that XLIFF supports ICU expressions).
Example previously {{expr}}, {{ expr }} and {{expr2}} had different ids but the exact same content once extracted. With this fix those 3 messages will only be extracted once with the same id.
We also aligned the digest functions for all serializers, they will all use the decimal digest (instead of sha1 previously for xliff).

Because of the bug #15573 we had to make a breaking change in the way i18n ids are generated.
To mitigate the problem, a new flag can be used to set the version of the digest method used: --i18nVersion with values 0 (old, default for v4) or 1 (new, default for v5) can be used by the cli tools for extraction (ng-xi18n) and AOT (ngc). The token I18N_VERSION can be used in your code for JIT.
We also added a new migration cli tool in @angular/compiler-cli named ng-migrate-i18n-0to1.
Usage is the following: ng-migrate-i18n-0to1 --files src/i18n/*.xlf --format xlf
--files: (required) the translation files that you want to migrate, you can use glob expressions to migrate multiple files at the same time (as long as they use the same format)
--format: (required) the format of the translation files that you want to migrate
--mapping: (optional) if you add this option it will generate a json file of mappings {newId --> [oldIds]} instead of migrating the translation files
--resolve: (optional) "manual" by default, you can use "auto" so that the cli tool doesn't ask when it finds conflicts (in which case it will take the first value)

Does this PR introduce a breaking change?

Yes

Unfortunately this is a breaking change because the auto-generated ids for extracted messages will change if the message contains placeholders (or ICU expressions for XLIFF)...
To mitigate the problem, a new flag can be used to set the version of the digest method used. It is --i18nVersion with values 0 (old, default for v4) or 1 (new, default for v5) for ngc, and the token I18N_VERSION for JIT.
For now we still use the old method to generate the ids, but once v5 is released we will reverse the flag to only use the new version.

vicb · 2017-06-22T20:50:52Z

packages/compiler-cli/src/i18n_options.ts

Should we throw in this case ?

change param to i18nFormat: string | null ?

vicb · 2017-06-22T20:51:35Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

nit: this is not the startOfEl - add a comment ?

better: extract content.lastIndexOf('>', sources[0].startSourceSpan !.start.offset) + 1 to an helper function to factorize, add doc there

vicb · 2017-06-22T20:55:02Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

What are start and end used for ?
Would your logic work if source elements are not back to back

<source>1</source> <other> <source>2</source>

good point, it would fail :(

vicb · 2017-06-22T20:56:29Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

Unused but required since we extend ml.RecursiveVisitor.

why would it be required ?

vicb · 2017-06-22T20:57:10Z

packages/compiler/test/i18n/migration/v0_to_v1_spec.ts

using remove for something with is not removed is a bad idea - old1 & old2 ?

be consistent why mapping and an inline mapping ? use v0ToV1 and v1ToV0

vicb · 2017-06-22T20:58:10Z

packages/compiler/test/i18n/migration/v0_to_v1_spec.ts

move this at describe level to avoir repeating ?

vicb · 2017-06-22T21:06:29Z

packages/compiler-cli/integrationtest/tsconfig-xi18n.json

why do we have to change the tsconfig here ?

because of a change that @tbosch made on master, without these parameters the tests won't work

vicb · 2017-06-22T21:07:53Z

packages/compiler-cli/src/migration/v0_to_v1.ts

I'd rather have execute and generateMapping()

vicb · 2017-06-22T21:09:02Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

[...sources] ?

vicb · 2017-06-22T21:09:48Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

what is the rationale here ?

vicb · 2017-06-26T23:06:29Z

packages/compiler-cli/src/i18n_options.ts

vicb · 2017-06-26T23:06:36Z

packages/compiler-cli/src/i18n_options.ts

vicb · 2017-06-26T23:06:47Z

packages/compiler-cli/src/i18n_options.ts

vicb · 2017-06-26T23:06:54Z

packages/compiler-cli/src/i18n_options.ts

vicb · 2017-06-26T23:14:37Z

packages/compiler-cli/integrationtest/src/mapping_v1_to_v0.json

Did you create an issue for this bug as discussed in the shared doc ? (startCol = endCol)

vicb · 2017-06-26T23:15:48Z

packages/compiler-cli/src/i18n_options.ts

remove ? from here for consistency

vicb · 2017-06-26T23:17:32Z

packages/compiler-cli/src/migration/v0_to_v1.ts

vicb · 2017-06-27T02:00:22Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

Should be el.visit(this, null);

vicb · 2017-06-27T02:01:42Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

sourcesInfos -> srcElements ?

vicb · 2017-06-27T02:03:02Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

.find -> .some

vicb · 2017-06-27T02:03:11Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

.find -> .some

vicb · 2017-06-27T02:03:55Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

sources -> srcElements

probably you should merge SourceInfosVisitor with MsgInfosVisitor ?

vicb · 2017-06-27T02:10:45Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

wouldn't replacing the id (right above) shift the sourcespan for the sources ?
If yes, why is this not caught by the tests.

because in my test the "old" id and the "new" id have the same length. Replacing "old" by "old2" made the tests fail

vicb · 2017-06-27T02:13:59Z

packages/compiler/src/i18n/migration/v0_to_v1.ts

maybe use constants here const XLIFF_MSG_EL = 'trans-unit';

googlebot · 2017-06-30T16:23:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mary-poppins · 2017-06-30T16:34:59Z

You can preview 6409883 at https://pr17638-6409883.ngbuilds.io/.

fixes angular#15573 `I18nVersion.Version1` fixes several bugs in the serializers. `I18nVersion.Version0` is identical to the behavior prior to this PR.

`i18nVersion` could be '0' or '1'

using `-i18nVersion=1`

…tent by Id

mary-poppins · 2017-07-17T09:35:43Z

You can preview 9d366bb at https://pr17638-9d366bb.ngbuilds.io/.

ocombe · 2017-11-03T08:50:27Z

Closing this for now because it's completely out of date and our code has changed too much. We'll have to start over if we want to fix this problem.

angular-automatic-lock-bot · 2019-09-12T19:52:47Z

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 area: i18n Issues related to localization and internationalization breaking changes action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 20, 2017

ocombe requested a review from vicb June 20, 2017 16:54

googlebot added the cla: no label Jun 20, 2017

ocombe mentioned this pull request Jun 20, 2017

fix(compiler): fix i18n IDs generation, add a migration tool #15621

Closed

3 tasks

vicb reviewed Jun 22, 2017

View reviewed changes

packages/compiler/test/i18n/migration/v0_to_v1_spec.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this at describe level to avoir repeating ?

vicb reviewed Jun 22, 2017

View reviewed changes

packages/compiler-cli/src/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have execute and generateMapping()

vicb reviewed Jun 22, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...sources] ?

vicb reviewed Jun 22, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the rationale here ?

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/i18n_options.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert ?

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/i18n_options.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert ?

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/i18n_options.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove ?

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/i18n_options.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove "?"

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/i18n_options.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove ? from here for consistency

vicb reviewed Jun 26, 2017

View reviewed changes

packages/compiler-cli/src/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

vicb reviewed Jun 27, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be el.visit(this, null);

vicb reviewed Jun 27, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourcesInfos -> srcElements ?

vicb reviewed Jun 27, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.find -> .some

vicb reviewed Jun 27, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.find -> .some

vicb reviewed Jun 27, 2017

View reviewed changes

packages/compiler/src/i18n/migration/v0_to_v1.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use constants here const XLIFF_MSG_EL = 'trans-unit';

ocombe mentioned this pull request Jun 30, 2017

MessageSpan contain the same value for startCol & endCol #17881

Closed

angular deleted a comment from mary-poppins Jun 30, 2017

ocombe and others added 15 commits July 17, 2017 11:09

refactor(compiler): add a version to the i18n serializers

12d930c

fixes angular#15573 `I18nVersion.Version1` fixes several bugs in the serializers. `I18nVersion.Version0` is identical to the behavior prior to this PR.

test(compiler): add tests for i18n serializers V1

a3c6a25

refactor(compiler): add a version parameter for i18n extraction

ed6b306

`i18nVersion` could be '0' or '1'

test(compiler): add integration tests for extraction

2f07c4a

using `-i18nVersion=1`

refactor(compiler): misc i18n refactoring

8ac8b82

feat(compiler): generate a mapping from i18n v1 id to v0 ids

6a4960a

feat(compiler): add an @internal function to return the message con…

f9e9d51

…tent by Id

feat(compiler): compute i18n conflicts

d297238

refactor(compiler): ApplyMapping - to finish

c572371

feat(compiler): resolve i18n migration conflicts

e3120af

feat(compiler-cli): create a migrate_i18n_0to1 script

e4d7184

feat(compiler-cli): add ngtools_api entry for the migration script

acce4cb

feat(core): adding I18N_VERSION token for jit compiler

0d9a67a

feat(compiler-cli): adding i18nVersion to ngc

0f842fd

feat(compiler-cli): adding mapping option for migration tool

ccb2ae9

ocombe force-pushed the i18n-version branch from 6409883 to 9d366bb Compare July 17, 2017 09:24

mhevery force-pushed the master branch from 08b36cb to 5d4b36f Compare July 20, 2017 21:49

mhevery force-pushed the i18n-version branch 3 times, most recently from a3bb636 to ccb2ae9 Compare July 26, 2017 18:06

ocombe added the state: blocked label Sep 7, 2017

ocombe closed this Nov 3, 2017

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

Conversation

ocombe commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jun 30, 2017

Uh oh!

mary-poppins commented Jun 30, 2017

Uh oh!

mary-poppins commented Jul 17, 2017

Uh oh!

ocombe commented Nov 3, 2017

Uh oh!

ocombe commented Jun 20, 2017 •

edited

Loading

vicb Jun 22, 2017 •

edited

Loading

vicb Jun 22, 2017 •

edited

Loading

vicb Jun 26, 2017 •

edited

Loading