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

feat(ivy): allow the locale to be set via a global property #33314

Closed
wants to merge 8 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 22, 2019

The ivy compile time inlining tools work on the code after it has
been compiled and bundled (potentially minified and mangled).
It is only at this point in the processing that the current locale
can be specified.

// FW-1639

See angular/angular-cli#15896

@petebacondarwin petebacondarwin added feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n target: major This PR is targeted for the next major release comp: ivy labels Oct 22, 2019
@petebacondarwin petebacondarwin requested a review from a team as a code owner October 22, 2019 08:32
@ngbot ngbot bot modified the milestone: needsTriage Oct 22, 2019
@AndrewKushnir
Copy link
Contributor

We discussed possible solutions with @mhevery and the proposal is to use $localize.locale as a way to communicate current locale to runtime code. The benefit of this approach is to have all localize-related logic grouped together under $localize. The processing of the $localize.locale might be different for compile-time inlining and runtime translations:

  • for compile-time inlining, we can just replace $localize.locale with a string that represents current locale, so it would not be present in the final bundle, similar to $localize tokens
  • for runtime translations, the $localize function would be available, so we can set the locale property to the current locale by adding additional code (in i18n inline logic or through CLI?)

The _localeFactory function (that is used to define the LOCALE_ID value) may access $localize.locale to get current locale (in the necessary branch of the _localeFactory function).

@petebacondarwin please let us know your opinion.

Thank you.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

See comment here: #33314 (comment). Thank you.

@petebacondarwin petebacondarwin force-pushed the global-locale branch 2 times, most recently from 7e0c98d to 129a065 Compare October 23, 2019 21:36
@petebacondarwin petebacondarwin requested a review from a team as a code owner October 23, 2019 21:36
"de": "src/locales/messages.de.json"
},
"sourceLocale": "en"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just preparatory - we are not yet using the built-in CLI inlining because it does not do the locale-inlining that this PR adds. @clydin - the CLI will need to be updated after this PR.

@petebacondarwin petebacondarwin force-pushed the global-locale branch 3 times, most recently from aa1e0ca to 4b52ec0 Compare October 23, 2019 21:49
* required, or just to provide their own `LOCALE_ID` provider.
*/
export function getGlobalLocale(): string {
return (ngI18nClosureMode && typeof goog !== 'undefined' && goog.LOCALE !== 'en') ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we making sure that goog.LOCALE is not en?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a comment would be useful explaining that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mhevery there is a comment about that in function description. It used to be above that specific line, now it's a bit more subtle, so may be we should move it back (closer to that line of code).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @petebacondarwin.


@Component(
{selector: 'app-root', templateUrl: './app.component.html', styleUrls: ['./app.component.css']})
export class AppComponent {
constructor(@Inject(LOCALE_ID) public locale: string) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for adding this check!

@AndrewKushnir
Copy link
Contributor

@petebacondarwin I've started g3 presubmits:

But it looks like there is a couple failing tests (related to i18n) in integration_test job in CircleCI - could you please look into this? Please let me know if fixes would require new presubmit.

Thank you.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit 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 Oct 23, 2019
The integration test now checks that the locale inlining is working.
@petebacondarwin petebacondarwin force-pushed the global-locale branch 2 times, most recently from e98e622 to 35fbd2c Compare October 24, 2019 08:41
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker 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: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker labels Oct 24, 2019
@petebacondarwin
Copy link
Member Author

Agh! The runtime translation setup added loads to the bundle sizes!
Cleaning that up...

$localize.locale = 'fr';
import {registerLocaleData} from '@angular/common';
import localeFr from '@angular/common/locales/fr';
registerLocaleData(localeFr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a lot like our previous CLI transform to add the locale:
https://github.com/angular/angular-cli/blob/99c174c03b101f0118dfb647313fc7a8d5233446/packages/ngtools/webpack/src/transformers/register_locale_data_spec.ts#L28-L44

I imagine it will be added in the same way. Probably in main.ts too, as the polyfills file is not guaranteed to exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, when we move over to using the CLI for all the translation stuff then I would expect this to be done by the CLI. But we need to tidy up the locale-id loose ends before we can do that.

I put the calls here because they had to be done before the bootstrap and I preferred to swap out this file than the main.ts in my configuration for runtime translation (since the $localize.locale is probably best done here).

"main-es2015": 128258,
"polyfills-es2015": 42102
"main-es2015": 138032,
"polyfills-es2015": 37494
Copy link
Member Author

Choose a reason for hiding this comment

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

The polyfills.ts size is lower because I removed the runtime translations from it. The production build would inline them instead.

@@ -30,8 +30,8 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 128258,
"polyfills-es2015": 42102
"main-es2015": 138032,
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is bigger because we added to the application component's template: the date pipe and the locale id.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 24, 2019
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Oct 24, 2019
@AndrewKushnir
Copy link
Contributor

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

AndrewKushnir pushed a commit that referenced this pull request Oct 24, 2019
In the post-$localize world the current locale value is defined by setting
`$localize.locale` which is then read at runtime by Angular in the provider
for the `LOCALE_ID` token and also passed to the ivy machinery via`setLocaleId()`.

The $localize compile-time inlining tooling can replace occurrences of
`$localize.locale` with a string literal, similar to how translations
are inlined.

// FW-1639

See angular/angular-cli#15896

PR Close #33314
AndrewKushnir pushed a commit that referenced this pull request Oct 24, 2019
During compile-time translation inlining, the `$localize.locale`
expression will now be replaced with a string literal containing the
current locale of the translations.

PR Close #33314
AndrewKushnir pushed a commit that referenced this pull request Oct 24, 2019
The integration test now checks that the locale inlining is working.

PR Close #33314
@petebacondarwin petebacondarwin deleted the global-locale branch October 24, 2019 18:04
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Oct 24, 2019
This commit also ensures that the integration tests
are all using the top level dependencies.

Resolves angular#33314 (comment)
AndrewKushnir pushed a commit that referenced this pull request Oct 24, 2019
This commit also ensures that the integration tests
are all using the top level dependencies.

Resolves #33314 (comment)

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

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 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants