build(common): add global UMD export to locale files #33504
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petebacondarwin
added
area: build & ci
petebacondarwin
force-pushed
the
locale-global-umd
branch
2 times, most recently
from
c98f5b1
to
501240a
Compare
In order to support adding locales during compile-time inlining of translations (i.e. after the TS build has completed), we need to be able to attach the locale to the global scope. This commit modifies the UMD wrapper of the final locale files that appear in the `@angular/common` package to export the locale onto the global object if not in AMD or CommonJS. The locale (e.g 'en-UK') is adding to the `ng.common.locale['en-UK']` global.
petebacondarwin
force-pushed
the
locale-global-umd
branch
from
501240a
to
1e1a0cb
Compare
To support compile time localization, we need to be able to provide the locales via a well known global property `ng.common.locale`. This commit changes `findLocaleData()` so that it will attempt to read the local from the global if the locale has not already been registered.
clydin
reviewed
Oct 31, 2019
|
|
||
| if (typeof global.ng === 'undefined') global.ng = {}; | ||
| if (typeof global.ng.common === 'undefined') global.ng.common = {}; | ||
| if (typeof global.ng.common.locale === 'undefined') global.ng.common.locale = {}; |
There was a problem hiding this comment.
Should these checks just bail if the locale object is not present?
How strict should these checks be? If someone decides to set ng.common.locale to null or 5, for instance.
There was a problem hiding this comment.
Yes, you are right - we should just bail. And we don't need to worry about using typeof to prevent crashes because the global is guaranteed to exist.
| // The locale names on the global object are not normalized, so we have to do a search. | ||
| // This is only once per requested locale; after that it is cached on LOCALE_DATA. | ||
| // Also generally only one or very few locales should be loaded onto the global. | ||
| for (const l in global.ng.common.locale) { |
There was a problem hiding this comment.
Suggested change
| for (const l in global.ng.common.locale) { | |
| for (const l of Object.keys(global.ng.common.locale)) { |
Probably won't be a problem but for..in walks the prototype chain. Object.entries would probably be cleaner overall but it would require a polyfill.
mhevery
suggested changes
Oct 31, 2019
There was a problem hiding this comment.
Rather than exporting ɵLOCALE_DATA from the @angular/core could we:
- export
clearLocaleData()method and use it in theafterAllfunctions - export
getLocaleDataasɵgetLocaleDatain case it is needed. (optional) - move
registerLocaleDatafrom@angular/commonto@angular/coreand than export it asɵregisterLocaleData. - Have
@angular/commonre-exportɵregisterLocaleDataasregisterLocaleDataso that it maintains backwards compatibility with public API.
| @@ -13,6 +13,7 @@ import localeAr from '@angular/common/locales/ar'; | |||
| import localeDeAt from '@angular/common/locales/de-AT'; | |||
| import {registerLocaleData, CurrencyPipe, DecimalPipe, PercentPipe, formatNumber} from '@angular/common'; | |||
| import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal'; | |||
| import {ɵLOCALE_DATA} from '@angular/core'; | |||
There was a problem hiding this comment.
Rather than exporting ɵLOCALE_DATA from the @angular/core could we:
- export
clearLocaleData()method and use it in theafterAllfunctions - export
getLocaleDataasɵgetLocaleDatain case it is needed. (optional) - move
registerLocaleDatafrom@angular/commonto@angular/coreand than export it asɵregisterLocaleData. - Have
@angular/commonre-exportɵregisterLocaleDataasregisterLocaleDataso that it maintains backwards compatibility with public API.
AndrewKushnir
added
target: patch
|
I am leaning towards the approach in #33523 instead... so I might close this one. |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to support adding locales during compile-time
inlining of translations (i.e. after the TS build has completed),
we need to be able to attach the locale to the global scope.
This commit modifies the UMD wrapper of the final locale files
that appear in the
@angular/commonpackage to export thelocale onto the global object if not in AMD or CommonJS.
There is no capacity in the TS compiler to modify the UMD wrapper
that it exports. This commit continues the current approach of modifying
the files before they get added to the npm package.
This is slightly brittle to any change that TS might make to their UMD output.
We could mitigate this by making the regex more flexible (but more complex)
or we could move completely to a new tool (like rollup) for generating the UMD
wrapper for each file.
To support compile time localization, we need to be
able to provide the locales via a well known global property
ng.common.locale.
This commit changes findLocaleData() so that it will
attempt to read the local from the global if the locale
has not already been registered.
I had to touch a number of tests because they were registering locales
and leaving them in place, which affected subsequent tests.