Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Implement locale message caching. #20

Merged
merged 3 commits into from
Oct 18, 2016
Merged

Conversation

mwistrand
Copy link
Contributor

Resolves #19. As noted in the issue, the purpose for this PR is to facilitate incorporation into dojo-widgets.

Cache locale-specific message dictionaries after they have been loaded,
returning those cached objects on subsequent calls to `i18n`. Also add a
`getCachedMessages` function that returns caches locale dictionaries
synchronously, with an additional `invalidate` function for clearing
bundle caches. Finally, after a locale dictionary has been resolved,
freeze it so it cannot be modified.
const cached = bundleMap[bundlePath];

if (cached) {
return cached[locale] as T;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the as T, the TS compiler warns that Messages cannot be assigned to type void | T; hence the casting.

@codecov-io
Copy link

codecov-io commented Oct 16, 2016

Current coverage is 99.24% (diff: 100%)

Merging #20 into master will increase coverage by 0.14%

@@             master        #20   diff @@
==========================================
  Files             2          2          
  Lines           111        133    +22   
  Methods           1          1          
  Messages          0          0          
  Branches         22         26     +4   
==========================================
+ Hits            110        132    +22   
  Misses            1          1          
  Partials          0          0          

Powered by Codecov. Last update c997417...6ed2256

Within `getCachedMessages`, use the bundle's supported `locales` to
determine whether the default messages should be returned, and to
normalize the locale to the most specific supported locale.
Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

A few thoughts...

@@ -90,6 +90,7 @@ export interface Messages {
const PATH_SEPARATOR: string = has('host-node') ? global.require('path').sep : '/';
const VALID_PATH_PATTERN = new RegExp(PATH_SEPARATOR + '[^' + PATH_SEPARATOR + ']+$');
const contextObjects: LocaleContext<LocaleState>[] = [];
let bundleMap = {} as BundleMap<Messages>;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use a Map here?

Copy link
Member

Choose a reason for hiding this comment

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

Or at least a Object.create(null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A Map would be more elegant here.

let localeCache = bundleMap[bundlePath];

if (!localeCache) {
localeCache = bundleMap[bundlePath] = {};
Copy link
Member

Choose a reason for hiding this comment

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

Again, maybe Object.create(null)?

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

👍

@mwistrand mwistrand merged commit 6ed2256 into dojo:master Oct 18, 2016
@dylans dylans added this to the 2016.10 milestone Oct 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants