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

ocombe · 2017-03-30T14:23:44Z

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.
Usage is the following: ng-migrate-i18n --files src/i18n/*.xlf --i18nFormat 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)
--i18nFormat: (required) the format of the translation files that you want to migrate
--i18nVersion: (optional) the version from which you are migrating. The only value for now is "0" (the new version is "1")
--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 iss --idVersion with values 0 (old, default for v4) or 1 (new, default for v5) for extraction (ng-xi18n) and AOT (ngc), and the token ID_VERSION for JIT.
Fow 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-03-30T16:43:57Z

packages/compiler/test/i18n/digest_spec.ts

Not sure if we should expect(sourceId) here

not really, I can remove it

vicb · 2017-03-30T16:45:45Z

packages/compiler/test/i18n/extractor_merger_spec.ts

Copy the former serializer to tests and use it. The expectations should not be modified here.

ahah damn, that's what I did the first time and then I thought that we should remove it because it was only used in the tests

vicb · 2017-03-30T16:46:15Z

packages/compiler/test/i18n/i18n_parser_spec.ts

same as previous file

vicb · 2017-03-30T16:47:30Z

packages/compiler/test/i18n/serializers/i18n_ast_spec.ts

same comment

vicb · 2017-03-30T16:47:49Z

packages/compiler/test/i18n/serializers/xtb_spec.ts

same comment

vicb · 2017-03-30T16:52:23Z

@ocombe thanks for the PR - I have added a few comments.

We need to come with a migration tool before merging this PR, can you work on it ?

vicb

see inline comments

ocombe · 2017-04-12T12:36:25Z

I changed the code that you requested @vicb, let me know if it's ok now

vicb · 2017-06-07T00:57:58Z

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

what is targetLocale here ?

good question... removing this

vicb · 2017-06-07T01:00:09Z

packages/compiler/test/i18n/serializers/xliff_spec.ts

revert (everywhere it has been added in this commit)

BREAKING CHANGE: updates the xliff serializer to use the same digest function. This is a breaking change because the IDs generated won't match with your existing translations. We provide a migration tool to update your existing files to the new method.

BREAKING CHANGE: This fix updates the digest function to ignore the content of placeholders. 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. This is a breaking change because the auto-generated ids for extracted messages will change if the message contains placeholders... To mitigate the problem, a migration tool will be provided. Fixes angular#15573

Because of the bug angular#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. Usage is the following: `ng-migrate-i18n --files src/i18n/*.xlf --i18nFormat 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) --i18nFormat: (required) the format of the translation files that you want to migrate --i18nVersion: (optional) the version from which you are migrating. The only value for now is "0" (the new version is "1") --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)

vicb · 2017-06-07T16:49:12Z

packages/compiler/src/i18n/digest.ts

  return computeMsgId(parts.join(''), message.meaning);
 }

+/**


We still depend on the placeholder name "ignoring placeholders" is not correct in that sense.

Could you please add more details ?

Please remove the comment about versions (version 1 here). Reason is digest and version are separate concern and this comment will become obsolete when the next version is out.

vicb · 2017-06-07T16:49:37Z

packages/compiler/src/i18n/digest.ts

+/**
+ * Serialize the i18n ast to something xml-like in order to generate an UID.
+ * Used by Id generation version 1
+ *


same comment as above for PH and version

vicb · 2017-06-07T17:12:38Z

packages/compiler/src/i18n/i18n_html_parser.ts


-function createSerializer(format?: string): Serializer {
+export function createSerializer(
+    format?: string, version: I18nVersion = I18nVersion.Version0): Serializer {


Why do we have a default parameter here ?
What about export function createSerializer(format: string, version: I18nVersion)
-> make both params mandatory

vicb · 2017-06-07T17:14:14Z

packages/compiler-cli/src/extractor.ts

-    return promiseBundle.then(bundle => {
-      const content = this.serialize(bundle, formatName);
+    return this.extractBundle().then((bundle: compiler.MessageBundle) => {
+      const serializer = compiler.createSerializer(formatName);


pass an explicit version here.
(Probably we need to think about making this parameter - let's discuss this)

vicb · 2017-06-07T17:57:46Z

packages/compiler-cli/src/extractor.ts

-      locale?: string|null, compilerHostContext?: CompilerHostContext,
-      ngCompilerHost?: CompilerHost): Extractor {
+      locale: string|null = null, compilerHostContext?: CompilerHostContext,
+      ngCompilerHost?: CompilerHost, version: I18nVersion = I18nVersion.Version0): Extractor {


changing the version here as no effect on extraction, right ?

(the version is only passed to the i18nHtmlParser that does not use it for extraction)

Yes, we always extract with the latest version, but this might be a mistake, shouldn't we let people use the old version for the extraction? in which case we should pass it here: https://github.com/ocombe/angular/blob/d8eaf6f215074ab4f9641dfd724da1b1909eaa99/packages/compiler-cli/src/extractor.ts#L37

yep, this is my comment above

vicb · 2017-06-07T18:04:04Z

packages/compiler/src/i18n/i18n_html_parser.ts

      private _htmlParser: HtmlParser, translations?: string, translationsFormat?: string,
      missingTranslation: MissingTranslationStrategy = MissingTranslationStrategy.Warning,
-      console?: Console) {
+      console?: Console, version: I18nVersion = I18nVersion.Version0) {


remove default value here = I18nVersion.Version0 ?

vicb · 2017-06-07T18:10:13Z

packages/compiler/src/i18n/migrator.ts

+  const msgIdToElement = getElementsMapping(translations);
+  msgIdToElement.reverse().forEach(([id, element]: [string, ml.Element]) => {
+    const newId = mapping[id];
+    if (typeof newId !== 'undefined' && newId !== id) {


if (mapping.hasOwnProperty(id)) { const newId = mapping[id]; // ... }

not sure if the test newId !== id is really helpful here (There is no reason why we should produce this anyway, right ?)

vicb · 2017-06-07T18:11:44Z

packages/compiler/src/i18n/migrator.ts

+
+/**
+ * Updates translations based on the provided mapping
+ * @param translations


rename translations or add a comment that it might also be message.

Do we have support to migrate an xmb file ?

yes we can migrate all formats (including xmb)

vicb · 2017-06-07T18:19:41Z

packages/compiler-cli/src/codegen.ts

+/**
+ * @internal
+ */
+export function getTranslations(i18nFile: string | null): string {


rename to readFileOrEmptyString

vicb · 2017-06-07T18:29:38Z

packages/compiler/src/i18n/migrator.ts

+  return {newToOld, oldToNew};
+}
+
+export function getElementsMapping(


what does this return ?
return an object ?
document ?

vicb · 2017-06-07T18:34:24Z

packages/compiler/src/i18n/migrator.ts

+
+  const res: [string, ml.Element, string][] = [];
+  msgIdToElement.forEach(([id, element]: [string, ml.Element]) => {
+    res.push([id, element, msgMap[id] ? msgMap[id] : '']);


.hasOwnProperty()

vicb · 2017-06-07T18:38:45Z

packages/compiler/src/i18n/migrator.ts

+    }
+  });
+
+  return {newToOld, oldToNew};


where do we need oldToNew

I think the names should be newIdToOldIds and oldIdToNewId (id vs ids is what is important here)

vicb · 2017-06-07T18:40:14Z

packages/compiler-cli/src/migrator.ts

+  private map(
+      mapping: {newToOld: {[newId: string]: string[]}, oldToNew: {[oldId: string]: string}},
+      i18nFile: string): string {
+    let dstPath = `${i18nFile.split('.').slice(0, -1).join('.')}_mapping.json`;


use path instead ?

we still need the name of the file to have a different mapping file for each file

path the package to manipulate paths

vicb · 2017-06-07T18:44:35Z

packages/compiler-cli/src/migrator.ts

+      mapping: {newToOld: {[newId: string]: string[]}, oldToNew: {[oldId: string]: string}},
+      i18nFile: string, formatName: string, version: I18nVersion,
+      resolve: string): Promise<string> {
+    const translations = getTranslations(i18nFile);


I would separate I/O (reading files, prompt) from processing.

It would still be nice to know conflicts even in the resolve mode (To display no conflict detected / N conflicts have been solved automatically)

more comments would be nice

Also probably the code related conflicts resolution should be in @angular/compiler
compiler-cli should only be orchestrating the calls & dealing with I/O

I would separate I/O (reading files, prompt) from processing.

can be done

It would still be nice to know conflicts even in the resolve mode (To display no conflict detected / N conflicts have been solved automatically)

I agree

more comments would be nice

everytime I add comments you make me remove them :D I'll add some

Also probably the code related conflicts resolution should be in @angular/compiler
compiler-cli should only be orchestrating the calls & dealing with I/O

it would complicate the code I think, the conflict resolution seems to be related to the prompts (for me) which are in the compiler-cli package

vicb · 2017-06-07T18:48:03Z

packages/compiler/src/config.ts


  constructor(
-      {defaultEncapsulation = ViewEncapsulation.Emulated, useJit = true, missingTranslation,
+      {defaultEncapsulation = ViewEncapsulation.Emulated, useJit = true, missingTranslation = null,


seems like this does not match the type ?

true, weird that the compiler didn't throw an error here

ocombe · 2017-06-20T16:55:18Z

Closed in favor of #17638

angular-automatic-lock-bot · 2019-09-11T22:21:01Z

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 Mar 30, 2017

ocombe assigned vicb Mar 30, 2017

ocombe requested review from marclaval and vicb March 30, 2017 14:23

googlebot added the cla: yes label Mar 30, 2017

ocombe force-pushed the fix-digest-15573 branch from adce335 to bc15bbf Compare March 30, 2017 14:33

vicb reviewed Mar 30, 2017

View reviewed changes

packages/compiler/test/i18n/i18n_parser_spec.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Mar 30, 2017

Choose a reason for hiding this comment

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

same as previous file

vicb reviewed Mar 30, 2017

View reviewed changes

packages/compiler/test/i18n/serializers/i18n_ast_spec.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Mar 30, 2017

Choose a reason for hiding this comment

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

same comment

vicb reviewed Mar 30, 2017

View reviewed changes

packages/compiler/test/i18n/serializers/xtb_spec.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Mar 30, 2017

Choose a reason for hiding this comment

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

same comment

vicb added state: blocked 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 Mar 30, 2017

vicb previously requested changes Mar 30, 2017

View reviewed changes

ocombe force-pushed the fix-digest-15573 branch from bc15bbf to d2acf58 Compare April 12, 2017 11:01

ocombe force-pushed the fix-digest-15573 branch from d2acf58 to 5e04000 Compare April 12, 2017 12:38

ocombe mentioned this pull request May 2, 2017

[i18n] plans #16477

Closed

20 tasks

ocombe force-pushed the fix-digest-15573 branch 8 times, most recently from 9a15596 to 7382687 Compare May 4, 2017 09:04

vicb reviewed Jun 7, 2017

View reviewed changes

ocombe force-pushed the fix-digest-15573 branch 4 times, most recently from 76deb4a to cd6d937 Compare June 7, 2017 12:51

ocombe force-pushed the fix-digest-15573 branch from cd6d937 to cbe3748 Compare June 7, 2017 13:15

ocombe force-pushed the fix-digest-15573 branch from cbe3748 to d8eaf6f Compare June 7, 2017 13:39

vicb reviewed Jun 7, 2017

View reviewed changes

ocombe closed this Jun 20, 2017

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

Conversation

ocombe commented Mar 30, 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!

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 commented Mar 30, 2017

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

ocombe commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jun 7, 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!

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 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ocombe commented Mar 30, 2017 •

edited

Loading

vicb Jun 7, 2017 •

edited

Loading

vicb Jun 7, 2017 •

edited

Loading

ocombe Jun 7, 2017 •

edited

Loading