Skip to content
This repository has been archived by the owner on Jan 29, 2024. It is now read-only.

Commit

Permalink
WIP implement registerStorageFactory
Browse files Browse the repository at this point in the history
  • Loading branch information
@0x-r4bbit
0x-r4bbit committed May 7, 2013
1 parent f1b5084 commit bbd9c4e
Show file tree
Hide file tree
Showing 4 changed files with 266 additions and 54 deletions.
2 changes: 1 addition & 1 deletion karma.conf.js
View file
Expand Up @@ -10,7 +10,7 @@ basePath = '';
files = [
JASMINE,
JASMINE_ADAPTER,
'bower_components/angular/angular.min.js',
'bower_components/angular/angular.js',
'bower_components/angular-cookies/angular-cookies.min.js',
'bower_components/angular-mocks/angular-mocks.js',
'ngTranslate/translate.js',
Expand Down
90 changes: 75 additions & 15 deletions ngTranslate/translate.js
View file
@@ -1,24 +1,24 @@
angular.module('ngTranslate', ['ng', 'ngCookies'])
angular.module('ngTranslate', ['ng'])

.run(['$translate', '$COOKIE_KEY', '$cookieStore', function ($translate, $COOKIE_KEY, $cookieStore) {
.run(['$translate', '$COOKIE_KEY', function ($translate, $COOKIE_KEY) {

if ($translate.rememberLanguage()) {
if (!$cookieStore.get($COOKIE_KEY)) {
if (!$translate.storage.get($COOKIE_KEY)) {

if (angular.isString($translate.preferredLanguage())) {
// $translate.uses method will both set up and remember the language in case it's loaded successfully
$translate.uses($translate.preferredLanguage());
} else {
$cookieStore.put($COOKIE_KEY, $translate.uses());
$translate.storage.set($COOKIE_KEY, $translate.uses());
}

} else {
$translate.uses($cookieStore.get($COOKIE_KEY));
$translate.uses($translate.storage.get($COOKIE_KEY));
}
} else if (angular.isString($translate.preferredLanguage())) {
$translate.uses($translate.preferredLanguage());
}

}]);

angular.module('ngTranslate').constant('$COOKIE_KEY', 'NG_TRANSLATE_LANG_KEY');
Expand All @@ -30,6 +30,7 @@ angular.module('ngTranslate').provider('$translate', function () {
$uses,
$rememberLanguage = false,
$missingTranslationHandler,
$storageFactoryFn,
$asyncLoaders = [],
NESTED_OBJECT_DELIMITER = '.';

Expand Down Expand Up @@ -102,7 +103,7 @@ angular.module('ngTranslate').provider('$translate', function () {
return $preferredLanguage;
}
};

this.uses = function (langKey) {
if (langKey) {
if (!$translationTable[langKey] && (!$asyncLoaders.length)) {
Expand All @@ -129,6 +130,52 @@ angular.module('ngTranslate').provider('$translate', function () {
$missingTranslationHandler = functionHandler;
};

this.registerStorageFactory = function (storageFactoryFn) {

if (!storageFactoryFn) {
throw new Error('Couldn\'t register storage factory without given storageFactoryFn!');
}

if (!(angular.isFunction(storageFactoryFn) || angular.isArray(storageFactoryFn))) {

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

Couldn't this just work like http interceptors? Just register a normal angular factory called ngTranslateLocalStorage or whatever - then if the user provides a string, it will try to fetch it like any other dependency.

You could also add shortcut methods like '.useLocalStorage()' or '.useCookieStore()'.

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Couldn't this just work like http interceptors? Just register a normal angular factory called ngTranslateLocalStorage or whatever -
then if the user provides a string, it will try to fetch it like any other dependency.

How should it try to fetch the given string as dependency, when there isn't a service or whatever under this name?

You could also add shortcut methods like '.useLocalStorage()' or '.useCookieStore()'.

Very good idea!

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

How should it try to fetch the given string as dependency, when there isn't a service or whatever under this name?

Well, the idea is you do make it a service :-)

angular.module('ngTranslate')

.factory('$translateLocalStorage', function() {
  return { set: function(){}, get: function() {} };
})
.factory('$translateCookieStorage', function() {
  // probably use $injector try/catch with $injector.get('$cookieStorage') to give a smart
  // error here if user doesn't include cookieStore - instead of just '$cookieStoreProvider not found'
  return { set: function(){}, get: function() {} };
})
myApp.config(function($translate) {
  //localstorage normal and shortcut
  $translateProvider.useStorage('$translateLocalStorage');
  $translateProvider.useLocalStorage();

  //OR cookies normal and shortcut
  $translateProvider.useStorage('$translateCookieStorage');
  $translateProvider.useCookieStorage()

  //OR custom
  $translateProvider.useStorage('mySuperheroStorage');
})

.factory('mySuperheroStorage', function() {
  return { set: function(){}, get: function() {} };
});

I think useStorage is a good name for the function: it's clear and short :-). It could also be setStorageFactory

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

This is actually pretty cool!

But I think, if we introduce a method like/called useStorage() or similar, rememberLanguage() becomes kinda waste, doesn't it?
Which means we could get rid of that then.

I really like the way of how to say which storage(factory) to use by passing a string, which is really available as a service. And it even works with custom services.

Hm.. but to make things straight forward, shouldn't then registerLoader() work similar? This is currently not possible. Which leads me to say, that registering storage factories should work like registering loaders...

Maybe I'm wrong. What do you guys say? @knalli, @ajoslin?

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

You could just make a separate .registerLoaderFactory() method, which takes anything $injector.get()-able.

Then for consistentcy you could call the storage .useStorageFactory(), which also takes anything $injector.get()-able.

And then have the presets still be .useCookieStorage() and .useLocalStorage()

Is the user able to register more than one loaderFactory? If not, I would call it useLoaderFactory as well.. use is shorter than register, and makes just as much sense :D

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

You're actually right. And since we're still under 1.0.0, introducing breaking changes, is okay. Actually.

Puh..

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

Puh indeed.

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Ahh.. I don't think that this works very well...

The registerLoader provides different behaviors, depending on what is passed.
This:

$translateProvider.registerLoader('url/to/endpoint');

Isn't actually a factory right? So we couldn't say something like useLoaderFactory().
Same goes for

$translateProvider.registerLoader({
    'prefix': 'locale-',
    'suffix': '.json',
    'type': 'static-files'
});

In that case, we have to at least passing some additional arguments, otherwise this wouldn't work with useLoaderFactory().

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

You could just have two methods though - registerLoader(): url or json, and registerLoaderFactory(): factory 😄

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Now this would be a pretty cool middleway. But.. ahh... to be honest, I'm not sure, if it's "okay" to introduce so many API, you know?

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

I think it's usually better two have more methods that do one thing than one method that can do anything. It's very clear this way. I guess the downside is that it is more to learn.

But it is kind of like angular this way. You have a .constant or a .factory. It could be this:

$translateProvider.loaderConstant(jsonOrStr);
$translateProvider.loaderFactory(factory);

that looks really angular-y.

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

True story.

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Okay guys, I'll set up a checklist to summarize the things we discussed on that commit. After that we can do some fine tuning on it and start with real implementation of all the things.
if (angular.isString(storageFactoryFn)) {

var cookieStorageFactoryFn = ['$cookieStore', function ($cookieStore) {
return {
get: function (name) { return $cookieStore.get(name); },
set: function (name, value) { $cookieStore.put(name, value); }
};
}];

switch (storageFactoryFn) {

case 'cookieStorage':
$storageFactoryFn = cookieStorageFactoryFn;
break;

case 'localStorage':

if ('localStorage' in window && window['localStorage'] !== null) {
$storageFactoryFn = ['$window', function ($window) {
return {
get: function (name) { return $window.localStorage.getItem(name); },

This comment has been minimized.

Sign in to view

Copy link
@knalli

knalli May 7, 2013

Member

We should support/use some kind of prefix for the data, so we

a) do not mix two apps using ng translate on the same domain
b) do not mess up an existing used store
c) support indirectly existing stuff

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Definitely.

This comment has been minimized.

Sign in to view

Copy link
@ajoslin

ajoslin May 8, 2013

$translateProvider.setStoragePrefix() ? $translateProvider.setStorageId()?

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Should there be a default prefix as well? So no one is required to use this method.
set: function (name, value) { $window.localStorage.setItem(name, value); }
};
}];
} else {
$storageFactoryFn = cookieStorageFactoryFn;

This comment has been minimized.

Sign in to view

Copy link
@DWand

DWand May 7, 2013

Member

Is this behavior predictable?

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

You mean the else case?

Yeah, when registering a storageFactory not by string ('cookieStorage', 'localStorage'), but by storageFactoryFn, sth. like:

$translateProvider.registerStorageFactory(['$http', function ($http) {
    return {
        get: function (name) {
        // do something with $http to get value
        },
        set: function (name, value) {
        // do something with $http to set value
        }
    };
}]);

This comment has been minimized.

Sign in to view

Copy link
@DWand

DWand May 8, 2013

Member

I mean another "else" case.
Is it predictable that the cookie storage will be used instead of local storage in case local storage is not available?

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Ha?
You could either say you want to use cookieStorage, localStorage (which falls back to cookieStorage if not available) or your own storage.

This comment has been minimized.

Sign in to view

Copy link
@DWand

DWand May 8, 2013

Member

Ok. Thanks for reply.
}
break;

default:
throw new Error('StorageFactory \'' + storageFactoryFn + '\' is not supported!');
break;
}
}
} else {
$storageFactoryFn = storageFactoryFn;
}
};

This comment has been minimized.

Sign in to view

Copy link
@knalli

knalli May 7, 2013

Member

As proposal good. These two would be the most used ones..

Using seriously, I would recommend extraction of the builders (just like the loaders one).

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

I'm okay with that
this.registerLoader = function (loader) {

if (!loader) {
Expand Down Expand Up @@ -210,11 +257,11 @@ angular.module('ngTranslate').provider('$translate', function () {
'$interpolate',
'$log',
'$injector',
'$cookieStore',
'$rootScope',
'$q',
'$COOKIE_KEY',
function ($interpolate, $log, $injector, $cookieStore, $rootScope, $q, $COOKIE_KEY) {
function ($interpolate, $log, $injector, $rootScope, $q, $COOKIE_KEY) {


var $translate = function (translationId, interpolateParams) {
var translation = ($uses) ?
Expand All @@ -233,10 +280,23 @@ angular.module('ngTranslate').provider('$translate', function () {
return translationId;
};

if ($rememberLanguage) {

if (!$storageFactoryFn) {
throw new Error('Couldn\'t remember language, no storage factory registered!');
}

$translate.storage = $injector.invoke($storageFactoryFn);

if (!$translate.storage.get && !$translate.storage.set) {

This comment has been minimized.

Sign in to view

Copy link
@DWand

DWand May 7, 2013

Member

Maybe, it's better to be an OR condition here?

This comment has been minimized.

Sign in to view

Copy link
@0x-r4bbit

0x-r4bbit May 8, 2013

Author Member

Ah, of course!
throw new Error('Registered storage doesn\'t implement storage definition!');
}
}

$translate.preferredLanguage = function() {
return $preferredLanguage;
return $preferredLanguage;
};

$translate.uses = function (key) {

if (!key) {
Expand All @@ -250,7 +310,7 @@ angular.module('ngTranslate').provider('$translate', function () {
$uses = key;

if ($rememberLanguage) {
$cookieStore.put($COOKIE_KEY, $uses);
$translate.storage.set($COOKIE_KEY, $uses);
}
$rootScope.$broadcast('translationChangeSuccess');
deferred.resolve($uses);
Expand All @@ -264,7 +324,7 @@ angular.module('ngTranslate').provider('$translate', function () {
$uses = key;

if ($rememberLanguage) {
$cookieStore.put($COOKIE_KEY, $uses);
$translate.storage.set($COOKIE_KEY, $uses);
}

deferred.resolve($uses);
Expand Down
102 changes: 64 additions & 38 deletions test/unit/translateServiceSpec.js
View file
Expand Up @@ -105,7 +105,7 @@ describe('ngTranslate', function () {
});
});

describe('$translateService (multi-lang)', function () {
describe('$translateService#rememberLanguage()', function () {

beforeEach(module('ngTranslate', function ($translateProvider) {
$translateProvider.translations('de_DE', {
Expand All @@ -119,8 +119,70 @@ describe('ngTranslate', function () {
$translateProvider.translations('en_EN', {
'YET_ANOTHER': 'Hello there!'
});
$translateProvider.uses('de_DE');
$translateProvider.preferredLanguage('de_DE');

$translateProvider.registerStorageFactory('cookieStorage');
$translateProvider.rememberLanguage(true);
}));

var $translate, $rootScope, $compile;

beforeEach(inject(function (_$translate_, _$rootScope_, _$compile_) {
$translate = _$translate_;
$rootScope = _$rootScope_;
$compile = _$compile_;
}));

it('should have a method rememberLanguage()', function () {
inject(function ($translate) {
expect($translate.rememberLanguage).toBeDefined();
});
});

it('should be a function', function () {
inject(function ($translate) {
expect(typeof $translate.rememberLanguage).toBe('function');
});
});

it('should return true if remmemberLanguage() is set to true', function () {
inject(function ($translate) {
expect($translate.rememberLanguage()).toBe(true);
});
});

it('should use fallback language if no language is stored in $cookieStore', function () {
inject(function ($cookieStore, $COOKIE_KEY) {
expect($cookieStore.get($COOKIE_KEY)).toBe('de_DE');
});
});

it('should remember when the language switched', function () {
inject(function ($translate, $rootScope, $cookieStore, $COOKIE_KEY) {
$cookieStore.remove($COOKIE_KEY);
expect($translate('YET_ANOTHER')).toBe('Hallo da!');
$translate.uses('en_EN');
$rootScope.$digest();
expect($translate('YET_ANOTHER')).toBe('Hello there!');
expect($cookieStore.get($COOKIE_KEY)).toBe('en_EN');
});
});
});

describe('$translateService (multi-lang)', function () {

beforeEach(module('ngTranslate', function ($translateProvider) {
$translateProvider.translations('de_DE', {
'EXISTING_TRANSLATION_ID': 'foo',
'ANOTHER_ONE': 'bar',
'TRANSLATION_ID': 'Lorem Ipsum {{value}}',
'TRANSLATION_ID_2': 'Lorem Ipsum {{value}} + {{value}}',
'TRANSLATION_ID_3': 'Lorem Ipsum {{value + value}}',
'YET_ANOTHER': 'Hallo da!'
});
$translateProvider.translations('en_EN', {
'YET_ANOTHER': 'Hello there!'
});
$translateProvider.preferredLanguage('en_EN');
$translateProvider.preferredLanguage('de_DE');
}));
Expand Down Expand Up @@ -198,42 +260,6 @@ describe('ngTranslate', function () {
});
});

describe('$translateService#rememberLanguage()', function () {

it('should have a method rememberLanguage()', function () {
inject(function ($translate) {
expect($translate.rememberLanguage).toBeDefined();
});
});

it('should be a function', function () {
inject(function ($translate) {
expect(typeof $translate.rememberLanguage).toBe('function');
});
});

it('should return true if remmemberLanguage() is set to true', function () {
inject(function ($translate) {
expect($translate.rememberLanguage()).toBe(true);
});
});

it('should use fallback language if no language is stored in $cookieStore', function () {
inject(function ($cookieStore, $COOKIE_KEY) {
expect($cookieStore.get($COOKIE_KEY)).toBe('de_DE');
});
});

it('should remember when the language switched', function () {
inject(function ($translate, $rootScope, $cookieStore, $COOKIE_KEY) {
expect($translate('YET_ANOTHER')).toBe('Hallo da!');
$translate.uses('en_EN');
$rootScope.$digest();
expect($translate('YET_ANOTHER')).toBe('Hello there!');
expect($cookieStore.get($COOKIE_KEY)).toBe('en_EN');
});
});
});

describe('$translateService#preferredLanguage()', function () {

Expand Down

0 comments on commit bbd9c4e

Please sign in to comment.