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

i18n: $localize message-id support #32594

Closed

Conversation

@petebacondarwin
Copy link
Member

commented Sep 10, 2019

Blocked on #32488

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

@googlebot googlebot added the cla: yes label Sep 10, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch from 3c169ff to a393cfc Sep 11, 2019
@@ -12,6 +12,7 @@ ts_library(
],
),
deps = [
"//packages/compiler",

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 11, 2019

Author Member

A question, which will become more pertinent as the compile-time stuff comes in:

What do we do about code that is needed in both @angular/localize and @angular/compiler?

In this PR we need to share the computeMsgId() function between both packages. Later we will need to share XML parsing utilities.

  1. I don't feel comfortable with either package depending upon the other.
  2. Equally it is not a good idea to duplicate the code in both package sources.
  3. It may be possible to do something clever with Bazel to have a single source Bazel package, which somehow gets bundled into both npm packages, but I have not worked out how to do this if it is possible.
  4. Having yet another package (e.g. @angular/compiler-utils) is tempting but would add to the proliferation of dependencies in the Angular core system.

Currently I have gone with 1) making @angular/localize dependent upon @angular/compiler.

@mhevery and @IgorMinar - any thoughts/ideas?

This comment has been minimized.

Copy link
@mhevery

mhevery Sep 11, 2019

Member

i like option #3. Could we do something with sim-links?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 13, 2019

Author Member

I don't think symlinks work well with Bazel ...

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch from a393cfc to 08ba8ed Sep 11, 2019
@ngbot ngbot bot modified the milestone: needsTriage Sep 11, 2019
@petebacondarwin petebacondarwin changed the title I18n fw 1538 message i18n: $localize message-id support Sep 11, 2019
@petebacondarwin petebacondarwin referenced this pull request Sep 11, 2019
0 of 14 tasks complete
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch 2 times, most recently from 83dcfe4 to 3f180e1 Sep 11, 2019
@petebacondarwin petebacondarwin marked this pull request as ready for review Sep 12, 2019
@petebacondarwin petebacondarwin requested review from IgorMinar and angular/dev-infra-framework as code owners Sep 12, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch from 1b36534 to 2051270 Sep 15, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

@AndrewKushnir - I had to rebase because of a conflict with master. Do you need to run a new presubmit?

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

@IgorMinar PTAL

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

@petebacondarwin if there are no major code changes, we can reuse previous TAP run. Thank you.

Copy link
Member

left a comment

left a bunch of comments, but overall lgtm

I am however growing more and more concerned about the amount of code we generate (which will affect dev mode).

packages/localize/init/index.ts Show resolved Hide resolved
packages/localize/src/translate.ts Outdated Show resolved Hide resolved
packages/localize/src/translate.ts Outdated Show resolved Hide resolved
packages/localize/src/utils/messages.ts Outdated Show resolved Hide resolved
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

I am however growing more and more concerned about the amount of code we generate (which will affect dev mode).

@IgorMinar - I too am concerned about this. The problem we have is that it is not possible to recreate the computed message-ids from the information that is available at the time we call $localize.

This could be avoided if we were to make breaking changes to the digest algorithms used to compute these message-ids. I believe that the spec (at least for XLIFF 1 and 2) does not specify the digest that should be used. The versions we use are specific to Angular - I believe somewhat based off some Google internal spec?? - so it is within our power to choose an algorithm that only cares about the message parts and placeholder names as available to $localize.

If we were to change our algorithms this would mean that all current message-id based translation files would be invalid and no longer match. It is feasible that we could provide a migration tool to move them to the new common algorithm.

By the way, you'll see (eventually once it is ready) that we have to do a whole load of processing in the compile-time inlining tool to convert these "external" message-ids to an "internal" format anyway for it to be able to work. I expect that fully functioning run-time translation that loads up translation files would need to do that same as well.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch from 2051270 to c12d377 Sep 16, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

Merge-assistance: Presubmit should be fine according to @AndrewKushnir (#32594 (comment))

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@petebacondarwin thanks for additional fixes. We've just landed big styling PR, so it'd be great to rebase to re-run CI with the most recent changes, could you please help with this?

I also started new TAP Presubmits:

If everything is "green", I'll proceed with merging this PR. Thank you.

This commit documents and extends the basic `$localize`
implementation to support adding a metadata block to the
start of a tagged message.

All the "pass-though" version does is to strip this block out,
similar to what it does to placeholder name blocks.
Previously the translation key used for translations was the `SourceMessage`
but it turns out that this is insufficient because "meaning" and "custom-id"
metadata affect the translation key.

Now run-time translation is keyed off the `MessageId`.
The `packages/localize/test/utils` folder was not being
included in the unit tests because the glob for the spec
files was only looking in the top level folder.
Now that the `$localize` translations are `MessageId` based the
compiler must render `MessageId`s in its generated `$localize` code.
This is because the `MessageId` used by the compiler is computed
from information that does not get passed through to the `$localize`
tagged string.

For example, the generated code for the following template

```html
<div id="static" i18n-title="m|d" title="introduction"></div>
```

will contain these localization statements

```ts
if (ngI18nClosureMode) {
  /**
    * @desc d
    * @meaning m
    */
  const MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1 = goog.getMsg("introduction");
  I18N_1 = MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1;
}
else {
  I18N_1 = $localize \`:m|d@@8809028065680254561:introduction\`;
}
```

Since `$localize` is not able to accurately regenerate the source-message
(and so the `MessageId`) from the generated code, it must rely upon the
`MessageId` being provided explicitly in the generated code.

The compiler now prepends all localized messages with a "metadata block"
containing the id (and the meaning and description if defined).

Note that this metadata block will also allow translation file extraction
from the compiled code - rather than relying on the legacy ViewEngine
extraction code. (This will be implemented post-v9).

Although these metadata blocks add to the initial code size, compile-time
inlining will completely remove these strings and so will not impact on
production bundle size.
As discussed in https://hackmd.io/33M5Wb-JT7-0fneA0JuHPA `SourceMessage`
strings are not sufficient for matching translations.

This commit updates `@angular/localize` to use `MessageId`s for translation
matching instead.

Also the run-time translation will now log a warning to the console if a
translation is missing.

BREAKING CHANGE:

Translations (loaded via the `loadTranslations()` function) must now use
`MessageId` for the translation key rather than the previous `SourceMessage`
string.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-FW-1538-message-id branch from c12d377 to dd1189f Sep 17, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

FYI, VE and Ivy Presubmits look normal, CI is green after rebase, PR is ready to be merged. Thank you.

AndrewKushnir added a commit that referenced this pull request Sep 17, 2019
…32594)

This commit documents and extends the basic `$localize`
implementation to support adding a metadata block to the
start of a tagged message.

All the "pass-though" version does is to strip this block out,
similar to what it does to placeholder name blocks.

PR Close #32594
AndrewKushnir added a commit that referenced this pull request Sep 17, 2019
)

Previously the translation key used for translations was the `SourceMessage`
but it turns out that this is insufficient because "meaning" and "custom-id"
metadata affect the translation key.

Now run-time translation is keyed off the `MessageId`.

PR Close #32594
AndrewKushnir added a commit that referenced this pull request Sep 17, 2019
As discussed in https://hackmd.io/33M5Wb-JT7-0fneA0JuHPA `SourceMessage`
strings are not sufficient for matching translations.

This commit updates `@angular/localize` to use `MessageId`s for translation
matching instead.

Also the run-time translation will now log a warning to the console if a
translation is missing.

BREAKING CHANGE:

Translations (loaded via the `loadTranslations()` function) must now use
`MessageId` for the translation key rather than the previous `SourceMessage`
string.

PR Close #32594
AndrewKushnir added a commit that referenced this pull request Sep 17, 2019
Now that the `$localize` translations are `MessageId` based the
compiler must render `MessageId`s in its generated `$localize` code.
This is because the `MessageId` used by the compiler is computed
from information that does not get passed through to the `$localize`
tagged string.

For example, the generated code for the following template

```html
<div id="static" i18n-title="m|d" title="introduction"></div>
```

will contain these localization statements

```ts
if (ngI18nClosureMode) {
  /**
    * @desc d
    * @meaning m
    */
  const MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1 = goog.getMsg("introduction");
  I18N_1 = MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1;
}
else {
  I18N_1 = $localize \`:m|d@@8809028065680254561:introduction\`;
}
```

Since `$localize` is not able to accurately regenerate the source-message
(and so the `MessageId`) from the generated code, it must rely upon the
`MessageId` being provided explicitly in the generated code.

The compiler now prepends all localized messages with a "metadata block"
containing the id (and the meaning and description if defined).

Note that this metadata block will also allow translation file extraction
from the compiled code - rather than relying on the legacy ViewEngine
extraction code. (This will be implemented post-v9).

Although these metadata blocks add to the initial code size, compile-time
inlining will completely remove these strings and so will not impact on
production bundle size.

PR Close #32594
@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-FW-1538-message-id branch Sep 25, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
The `packages/localize/test/utils` folder was not being
included in the unit tests because the glob for the spec
files was only looking in the top level folder.

PR Close angular#32594
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…ngular#32594)

This commit documents and extends the basic `$localize`
implementation to support adding a metadata block to the
start of a tagged message.

All the "pass-though" version does is to strip this block out,
similar to what it does to placeholder name blocks.

PR Close angular#32594
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…ular#32594)

Previously the translation key used for translations was the `SourceMessage`
but it turns out that this is insufficient because "meaning" and "custom-id"
metadata affect the translation key.

Now run-time translation is keyed off the `MessageId`.

PR Close angular#32594
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…2594)

As discussed in https://hackmd.io/33M5Wb-JT7-0fneA0JuHPA `SourceMessage`
strings are not sufficient for matching translations.

This commit updates `@angular/localize` to use `MessageId`s for translation
matching instead.

Also the run-time translation will now log a warning to the console if a
translation is missing.

BREAKING CHANGE:

Translations (loaded via the `loadTranslations()` function) must now use
`MessageId` for the translation key rather than the previous `SourceMessage`
string.

PR Close angular#32594
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…32594)

Now that the `$localize` translations are `MessageId` based the
compiler must render `MessageId`s in its generated `$localize` code.
This is because the `MessageId` used by the compiler is computed
from information that does not get passed through to the `$localize`
tagged string.

For example, the generated code for the following template

```html
<div id="static" i18n-title="m|d" title="introduction"></div>
```

will contain these localization statements

```ts
if (ngI18nClosureMode) {
  /**
    * @desc d
    * @meaning m
    */
  const MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1 = goog.getMsg("introduction");
  I18N_1 = MSG_EXTERNAL_8809028065680254561$$APP_SPEC_TS_1;
}
else {
  I18N_1 = $localize \`:m|d@@8809028065680254561:introduction\`;
}
```

Since `$localize` is not able to accurately regenerate the source-message
(and so the `MessageId`) from the generated code, it must rely upon the
`MessageId` being provided explicitly in the generated code.

The compiler now prepends all localized messages with a "metadata block"
containing the id (and the meaning and description if defined).

Note that this metadata block will also allow translation file extraction
from the compiled code - rather than relying on the legacy ViewEngine
extraction code. (This will be implemented post-v9).

Although these metadata blocks add to the initial code size, compile-time
inlining will completely remove these strings and so will not impact on
production bundle size.

PR Close angular#32594
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.