refactor(common): simplify closure-locale by removing imports #18926
Conversation
ocombe
added
area: i18n
state: blocked
target: major
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
You can preview a41365b at https://pr18926-a41365b.ngbuilds.io/. |
tools/gulp-tasks/cldr/closure.js
Outdated
| const yargs = require('yargs').argv; | ||
| const {I18N_FOLDER, I18N_DATA_FOLDER, RELATIVE_I18N_DATA_FOLDER, HEADER} = require('./extract'); | ||
| const OUTPUT_NAME = `closure-locale.ts`; | ||
|
|
||
| module.exports = (gulp, done) => { | ||
| let GOOG_LOCALES; | ||
| let GOOG_LOCALES = [ |
There was a problem hiding this comment.
explain what this table is. Should be const ?
There was a problem hiding this comment.
it's a "let" because it can be overwritten by cli arguments
tools/gulp-tasks/cldr/closure.js
Outdated
| 'sk', 'sl', 'sq', 'sr', 'sv', 'sw', 'ta', 'te', 'th', 'tl', | ||
| 'tr', 'uk', 'ur', 'uz', 'vi', 'zh', 'zh-CN', 'zh-HK', 'zh-TW', 'zu' | ||
| ]; | ||
| const ALIASES = { |
There was a problem hiding this comment.
explain what this table is.
tools/gulp-tasks/cldr/closure.js
Outdated
|
|
||
| l[0] = goog.LOCALE; |
There was a problem hiding this comment.
l might not be defined.
explain what you do with l[0] = goog.LOCALE
There was a problem hiding this comment.
also seems like goog.LOCALE is not normalized ? (ie - vs _)
tools/gulp-tasks/cldr/closure.js
Outdated
|
|
||
| // to avoid duplicated "case" we regroup all locales in the same "case" | ||
| // the simplest way to do that is to have alias aliases | ||
| // e.g. 'no' --> 'nb', 'nb' --> 'no-NO' |
There was a problem hiding this comment.
don't see the loop that does that ?
There was a problem hiding this comment.
We only have one level of alias aliases, no need for a loop right now. We'll complicate the code later if necessary
ocombe
changed the title
vicb
force-pushed
the
closure-locale-all
branch
from
a41365b
to
749dfbb
Compare
|
CLAs look good, thanks! |
|
You can preview 749dfbb at https://pr18926-749dfbb.ngbuilds.io/. |
ocombe
force-pushed
the
closure-locale-all
branch
from
749dfbb
to
456ecee
Compare
|
You can preview 456ecee at https://pr18926-456ecee.ngbuilds.io/. |
ocombe
force-pushed
the
closure-locale-all
branch
from
456ecee
to
1366e34
Compare
|
You can preview 1366e34 at https://pr18926-1366e34.ngbuilds.io/. |
vicb
added
action: merge
|
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. |
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The current closure locale contains too many imports which slows the build for g3
What is the new behavior?
We inlined all of the locales in the closure-locale file, and closure will remove all the unnecessary data at the end of the build.
We also only used the locales that closure uses instead of all of the locales available.
Does this PR introduce a breaking change?
Other information
Do not merge this PR before #18907