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
Conversation
We discussed possible solutions with @mhevery and the proposal is to use
The @petebacondarwin please let us know your opinion. Thank you. |
There was a problem hiding this 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.
4a66c14
to
3aca5a2
Compare
7e0c98d
to
129a065
Compare
"de": "src/locales/messages.de.json" | ||
}, | ||
"sourceLocale": "en" | ||
}, |
There was a problem hiding this comment.
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.
aa1e0ca
to
4b52ec0
Compare
* required, or just to provide their own `LOCALE_ID` provider. | ||
*/ | ||
export function getGlobalLocale(): string { | ||
return (ngI18nClosureMode && typeof goog !== 'undefined' && goog.LOCALE !== 'en') ? |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this 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) {} |
There was a problem hiding this comment.
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!
@petebacondarwin I've started g3 presubmits: But it looks like there is a couple failing tests (related to i18n) in Thank you. |
The integration test now checks that the locale inlining is working.
e98e622
to
35fbd2c
Compare
Agh! The runtime translation setup added loads to the bundle sizes! |
$localize.locale = 'fr'; | ||
import {registerLocaleData} from '@angular/common'; | ||
import localeFr from '@angular/common/locales/fr'; | ||
registerLocaleData(localeFr); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
35fbd2c
to
c251dc0
Compare
c251dc0
to
3b15764
Compare
"main-es2015": 128258, | ||
"polyfills-es2015": 42102 | ||
"main-es2015": 138032, | ||
"polyfills-es2015": 37494 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
FYI, VE and Ivy presubmits are successful. Thank you. |
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
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
The integration test now checks that the locale inlining is working. PR Close #33314
This commit also ensures that the integration tests are all using the top level dependencies. Resolves angular#33314 (comment)
This commit also ensures that the integration tests are all using the top level dependencies. Resolves #33314 (comment) PR Close #33382
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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