-
Notifications
You must be signed in to change notification settings - Fork 78
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
Update I18n modules to support loading automatically bundled CLDR data #657
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 449f5dc:
|
55135af
to
e3fc460
Compare
src/i18n/i18n.ts
Outdated
const localCldrLoader = cldrLoaders[userLocale]; | ||
if (localCldrLoader && localCldrLoader !== true) { | ||
loaderPromises.push(localCldrLoader()); | ||
} |
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.
Can this be changed from localCldrLoader
to localeCldrLoader
since it's specific to the locale and has nothing to do with the local
option?
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.
Yup absolutely
src/i18n/i18n.ts
Outdated
messages: Object.keys(bundle.messages).reduce( | ||
(messages, key) => { | ||
const message = globalize.cldr.get(`${MESSAGE_BUNDLE_PATH}/${bundleId}/${key}`); | ||
messages[key] = message; | ||
return messages; | ||
}, | ||
{} as any | ||
), |
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.
When called from within middleware/i18n.localize()
, will this object be regenerated each time the widget is invalidated? If so, caching the result would be nice.
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.
Done.
tests/core/unit/middleware/i18n.tsx
Outdated
root.innerHTML, | ||
'<div><div>{"foo":""}</div><div></div><div>true</div><div>{"locale":"es"}</div><button>es</button></div>' | ||
); | ||
es.resolver({ default: { foo: 'holla, {name}' } }); |
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.
Nit: "hello" in Spanish is "hola"
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.
Lol Thanks 😄
loadData(frCurrencies); | ||
loadData(frNumbers); | ||
loadData(frUnits); | ||
}); |
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.
Just verifying nothing throws?
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 used to load data in for the tests, runs in both browser and node.
ed6f373
to
f28b9b0
Compare
@mwistrand I think I've addressed your comments |
5ab6911
to
449f5dc
Compare
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
==========================================
- Coverage 98.06% 97.80% -0.26%
==========================================
Files 120 117 -3
Lines 6654 6570 -84
Branches 1510 1494 -16
==========================================
- Hits 6525 6426 -99
- Misses 129 144 +15
Continue to review full report at Codecov.
|
Type: feature
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
Comprehensive change of i18n support in Dojo; The motivation is to enable automatic detection of CLDR data requirements with lazy loading and resolution of i18n CLDR data bundles. Ensuring that CLDR data automatically included in i18n enables Dojo to remove existing custom locale resolution logic and entirely depend on
Globalize
for locale resolution.This should be backwards compatible, unless the downstream application is importing interfaces that are no longer needed (and can be removed) or using the core i18n module directly (which shouldn't be required).
Details:
Globalize
for all message bundle resolution.cldr
module that loads the required supplemental data (This import will automatically be elided during the Dojo build)Related to #655