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 - legacy message id changes #32867

Closed

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Sep 26, 2019

This PR implements Common digest function functionality of the legacy message-id support.

@googlebot googlebot added the cla: yes label Sep 26, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch from e220148 to 3d06e7e Sep 26, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch 2 times, most recently from 68e6f21 to 9c1d43f Sep 26, 2019
@atscott atscott added the comp: i18n label Sep 26, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 26, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch from 9c1d43f to bbd3d46 Sep 26, 2019
@petebacondarwin petebacondarwin marked this pull request as ready for review Sep 26, 2019
@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Sep 26, 2019
Copy link
Contributor

AndrewKushnir left a comment

LGTM 👍 Thanks Pete!

@AndrewKushnir AndrewKushnir requested a review from mhevery Sep 27, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Sep 27, 2019

Started presubmits to get early results:

@ocombe
ocombe approved these changes Sep 27, 2019
Copy link
Contributor

ocombe left a comment

I don't think that you should put a breaking change message in fix(ivy): i18n - do not render message ids unnecessarily, as it might appear in the v9 release changelog, even if you're not breaking anything that existed previously

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Sep 30, 2019

FYI, VE and Ivy presubmits are successful. Thank you.

Copy link
Member

IgorMinar left a comment

a few small changes requested, but otherwise looks good.

packages/localize/src/utils/messages.ts Outdated Show resolved Hide resolved
import { AppComponent } from './app.component';
// We must load translations before `AppComponent` is imported
// Otherwise the `$localize` calls inside its template will not be translated
import './translations';

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Oct 2, 2019

Member

we should not be relying on the fact that application code can have side-effecty module imports because build optimizations will likely break such code. (I know that this is just a test app, but some people will copy it). Please rewrite this into code that doesn't have side-effecty imports.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 2, 2019

Author Member

I'm not quite sure how I can do that without adding this to the polyfills.ts file.

What would be cool is if we could tell Angular CLI that there is a file that must be executed before the main bundle...

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Oct 2, 2019

Member

the main.ts file is one that must have side effects (the bootstrap call), so what I suggest is that you export configTranslations or loadTranslations fn from translations.ts, and then import and call it from main.ts.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 2, 2019

Author Member

Sadly that doesn't work because the call to loadTranslations() (the one from @angular/localize) must execute before the module loader even evaluates the @Component() decoration of AppComponent.

Any code that is inside the body of main.ts will be evaluated after all the imports have been evaluated - whatever order they actually occur in the file - and so putting any kind of code execution (e.g. calling configureTranslations() etc) in main.ts will fail to run early enough.

The only way for it to work is either to have a side-effect-y import like this or to put it into a bundle that will be executed before the main bundle (i.e. polyfills).

I am going to go with the latter since that is where we already put the import of @angular/localize/init anyway.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch from bbd3d46 to a41f9cf Oct 2, 2019
In an attempt to be compatible with previous translation files
the Angular compiler was generating instructions that always
included the message id. This was because it was not possible
to accurately re-generate the id from the calls to `$localize()` alone.

In line with https://hackmd.io/EQF4_-atSXK4XWg8eAha2g this
commit changes the compiler so that it only renders ids if they are
"custom" ones provided by the template author.

NOTE:

When translating messages generated by the Angular compiler
from i18n tags in templates, the `$localize.translate()` function
will compute message ids, if no custom id is provided, using a
common digest function that only relies upon the information
available in the `$localize()` calls.

This computed message id will not be the same as the message
ids stored in legacy translation files. Such files will need to be
migrated to use the new common digest function.

This only affects developers who have been trialling `$localize`, have
been calling `loadTranslations()`, and are not exclusively using custom
ids in their templates.
Metadata blocks are delimited by colons. Previously the code naively just
looked for the next colon in the string as the end marker.

This commit supports escaping colons within the metadata content.
The Angular compiler has been updated to add escaping as required.
…placeholder

Previously if a translation contains a placeholder that
does not exist in the message being translated, that
placeholder is evaluated as `undefined`.

Translations should never contain such placeholder names
so now `translate` will throw a helpful error in instead.
Previously the metadata and placeholder blocks were serialized in
a variety of places. Moreover the code for creating the `LocalizedString`
AST node was doing serialization, which break the separation of concerns.

Now this is all done by the code that renders the AST and is refactored into
helper functions to avoid repeating the behaviour.
The missing translation and invalid placeholder warnings now contain the
"meaning" if one was provided.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch from a41f9cf to a293d41 Oct 2, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-legacy-id branch from a293d41 to 89b5bc0 Oct 2, 2019
@atscott atscott closed this in 601f87c Oct 2, 2019
atscott added a commit that referenced this pull request Oct 2, 2019
The missing translation and invalid placeholder warnings now contain the
"meaning" if one was provided.

PR Close #32867
atscott added a commit that referenced this pull request Oct 2, 2019
)

Previously the metadata and placeholder blocks were serialized in
a variety of places. Moreover the code for creating the `LocalizedString`
AST node was doing serialization, which break the separation of concerns.

Now this is all done by the code that renders the AST and is refactored into
helper functions to avoid repeating the behaviour.

PR Close #32867
atscott added a commit that referenced this pull request Oct 2, 2019
Metadata blocks are delimited by colons. Previously the code naively just
looked for the next colon in the string as the end marker.

This commit supports escaping colons within the metadata content.
The Angular compiler has been updated to add escaping as required.

PR Close #32867
atscott added a commit that referenced this pull request Oct 2, 2019
In an attempt to be compatible with previous translation files
the Angular compiler was generating instructions that always
included the message id. This was because it was not possible
to accurately re-generate the id from the calls to `$localize()` alone.

In line with https://hackmd.io/EQF4_-atSXK4XWg8eAha2g this
commit changes the compiler so that it only renders ids if they are
"custom" ones provided by the template author.

NOTE:

When translating messages generated by the Angular compiler
from i18n tags in templates, the `$localize.translate()` function
will compute message ids, if no custom id is provided, using a
common digest function that only relies upon the information
available in the `$localize()` calls.

This computed message id will not be the same as the message
ids stored in legacy translation files. Such files will need to be
migrated to use the new common digest function.

This only affects developers who have been trialling `$localize`, have
been calling `loadTranslations()`, and are not exclusively using custom
ids in their templates.

PR Close #32867
atscott added a commit that referenced this pull request Oct 2, 2019
@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-legacy-id branch Oct 3, 2019
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…placeholder (angular#32867)

Previously if a translation contains a placeholder that
does not exist in the message being translated, that
placeholder is evaluated as `undefined`.

Translations should never contain such placeholder names
so now `translate` will throw a helpful error in instead.

PR Close angular#32867
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
The missing translation and invalid placeholder warnings now contain the
"meaning" if one was provided.

PR Close angular#32867
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…ular#32867)

Previously the metadata and placeholder blocks were serialized in
a variety of places. Moreover the code for creating the `LocalizedString`
AST node was doing serialization, which break the separation of concerns.

Now this is all done by the code that renders the AST and is refactored into
helper functions to avoid repeating the behaviour.

PR Close angular#32867
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
Metadata blocks are delimited by colons. Previously the code naively just
looked for the next colon in the string as the end marker.

This commit supports escaping colons within the metadata content.
The Angular compiler has been updated to add escaping as required.

PR Close angular#32867
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
In an attempt to be compatible with previous translation files
the Angular compiler was generating instructions that always
included the message id. This was because it was not possible
to accurately re-generate the id from the calls to `$localize()` alone.

In line with https://hackmd.io/EQF4_-atSXK4XWg8eAha2g this
commit changes the compiler so that it only renders ids if they are
"custom" ones provided by the template author.

NOTE:

When translating messages generated by the Angular compiler
from i18n tags in templates, the `$localize.translate()` function
will compute message ids, if no custom id is provided, using a
common digest function that only relies upon the information
available in the `$localize()` calls.

This computed message id will not be the same as the message
ids stored in legacy translation files. Such files will need to be
migrated to use the new common digest function.

This only affects developers who have been trialling `$localize`, have
been calling `loadTranslations()`, and are not exclusively using custom
ids in their templates.

PR Close angular#32867
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Nov 3, 2019

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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.