Use localStorage when available for rememberLanguage #3
Conversation
|
Thanks for the PR. Please consider the Contributing Guide. As you can see, your code failed on build process. Also, there aren't tests submitted at all. |
|
BTW: I just found this angular-local-storage module. Maybe we should consider either extracting the best bits out of it, and pack it in a custom service/provider for ngTranslate, or try contributing to angular-local-storage, so that it at least provides a test suite. |
|
+1 for setting up a kind of abstraction service, which decides to use localStorage service or $cookieStore. Better for testability, better to extend and maybe there's another way of store the language key, so it woudn't be a problem to add another store service. |
|
I know. I didn't mean for the PR to be finished, it's there for discussion and I haven't had time to even test it :) El 08/04/2013, a las 21:47, Pascal Precht notifications@github.com escribió:
|
|
Ah, okay my bad :) |
|
This feature is optional right? So perhaps the dependency can be optional as well? |
|
@thobias Actually you're totally right! I thought the same yesterday's eve, that the |
|
I just looked at the code, the problem is that there's no way to conditionally inject a module into angular runtime (at least as far as I've seen), so we can't check to see if we need to load the $cookieStore… I'm thinking the only "solution" to this is to leave this functionality out of the component and let users create their own cookie/localStorage-solution (which is what I would have done anyway). I think that's the cleanest solution, since it also makes the component more versatile (ie. it lets people use it how they want, and not force cookies/localStorage) and more focused. This also makes sense in larger project where the locale may be a property of the logged in user or such (in which case we already have a session with the locale in it). |
|
@thobias Totally agree with you, but I think there's a way to solve this.. I mean you could at least 'overwrite' $translate service with a ngTranslate extension module. I'll research to find a better solution on that. |
|
What if you just did something as simple as this: if (localStorageIsSupported) {
loadRememberSystem();
} else {
try {
var cookieStore = $injector.get('$cookieStore');
loadRememberSystemWithCookieStore(cookieStore);
catch (e) {
//no $cookieStore, no angular-cookies, no remember!
}
} |
|
As for me, words "setting up a kind of abstraction service" is exactly what is needed. I don't know how about JS and AnglularJS, but thinking out of the OOP paradigm we might simply provide a storage interface. So user will be able create custom class/service which realizes this interface (and use cookies, local storage, or whatever he wants). And also ngTranslate will not be longer depend of cookiesStorage - this dependensy will be moved to the implementation of the interface. |
|
Good ol' UML <3 Since we're not able to get service instances at So let's say somebody want to use any kind of storage to store a language key and let his app remember the configured language. A possible solution for that could be (similar to app.config(function ($translateProvider) {
// registering storage provider
$translateProvider.registerStorageProvider(['dep1', 'dep2', function ($dep1, $dep2) {
return {
get: function (key) { /* ... */ },
set: function (key, value) { /* ... */}
};
}]);
// make uses of it at run() phrase
$translateProvider.rememberLanguage(true);
});And later on at This gives the user full control of any kind of storage. It just has to implement the defined interface. |
|
@ajoslin ? |
|
Yep. Perhaps we can provide some nice built-ins where the transitive dependencies have to be managed outside? Just an idea, don't now if this will even work. angular.module('app', ['ngTranslate', 'cookieStore']).config(function($translateProvider){
$translateProvider.registerStore('cookie');
});Just like the XHRs, small bluprints. |
|
Yeah we could do so. :) |
|
Now, since this is actually a kinda breaking change, I wonder if this feature should land in 1.0.0 or if we should implement this for 0.7.0 or 0.8.0 or so. |
|
@PascalPrecht I like that idea. As @knalli said you can just have pre-registered factories for localStorage and cookies. And then the user can just register those if he wants to use them, depending on what browser he is in and if he's loaded cookies. Probably I wouldn't call it |
|
So, after all this discussion, my take on this is that forcing ANY interface on a storage/caching engine is not good. What if any other library needs caching too and requires another interface? I propose to let the user configure a "getter" and a "setter" function, which would just be used by the library if possible. In the lines of the following: (by default a no-op) angular.module('ngTranslate').value(
'translate.config.cache', {
get: function(){},
set: function(){}
}
);so the user could use (in their app) angular.module('myApp').value(
'translate.config.cache', {
get: function() {
return localStorage.get("LANG");
},
set: function(val) {
localStorage.set("LANG", val);
}
}
);and the library could provide their own implementations, as an optional module, so another user could use angular.module('myApp', ['ngTranslate', 'ngTranslate-rememberInCookie'])and the additional dependency would just override the |
I agree.
This is just where So in some cases, it could just be used as a facade-pattern. |
|
So, here is a WIP @knalli @ajoslin @DWand @rewritten bbd9c4e |
|
Regarding the proposed commit above, here's a checklist (which now has to be fine tuned):
Landed as 049512b
|
|
I had a thought for your loader registration function. You technically could have it every way (json, url, or string-dependency). function getLoader(value) {
if (angular.isArray(value) || angular.isFunction(value)) {
return $injector.invoke(value); //factory
} else if (angular.isObject(value)) {
return json(value); //json
} else if (angular.isString(value)) {
//if it's a string, it must be a dependency name or a url
try {
return $injector.get(value); //dependency name
} catch (e) {
//exception? our dependency doesn't exist. it must be a url!
return url(value);
}
}
} |
|
What if I do not want to use storage? Do we implement a NoOp/Null variant? |
|
You just don't use it :) On Thursday, May 9, 2013, Jan Philipp wrote:
/pp Sent from Gmail Mobile |
|
Hey! Just one more question )) |
|
And what is the difference between |
Yeah, we could actually do so. But I'd say we introduce this kind of feature when there's more you can configure. As long as we have just these few methods we don't need that configuration pattern. So in conclusion: Yes we'll do so, but not yet :)
The first is to make it possible to change the value which is stored in $STORAGE_KEY and the second is there to customize a specific prefix for the value of $STORAGE_KEY to avoid conflicts. |
|
|
|
well, bisect it, see my comment. the specified commit was the good one. ;) I can have a look at it later or tomorrow. |
|
okay, time was there. |
|
Yeah I know where it is and I know that's an issue with the $translateLocalStorage but I can't figure out, why it doesn't set the storageKey property on startup. |
|
Because the locale storage module is constructed before you set the language. And after you set the language, the locale storage is not being refreshed. |
|
Whoat? :-o |
|
Instrumented some breakpoints and the thing will be clear. |
|
Sorry I don't get it. So yeah, |
|
There are some errors in the current code. You use the $STORAGE_KEY instead of $storageKey. Maybe it might affect on results. Here is correct variant: 98028bb |
|
@DWand This is fixed now. |
|
I really can't get around this weird error. https://github.com/PascalPrecht/ng-translate/blob/canary/test/unit/translateServiceSpec.js#L624-L630 Anybody else have an idea whats wrong there? I also logged the code at different states. Really don't get it. |
|
So, the checklist is done. There is just this weird bug with |
|
Got it, test case wasn't solid cleaned up. |
|
With 625b1d6, works for me. |
|
beforeEach(function() {
var store = {};
spyOn(localStorage, 'getItem').andCallFake(function(key) {
return store[key];
});
spyOn(localStorage, 'setItem').andCallFake(function(key, value) {return store[key] = value + '';});
}); |
|
Since everything landed in canary now, this will be closed. Thanks for all your input and work. You guys are great! |
Still not tested, I don't have the right setup to test it now. If anyone can test it before I am able to, please comment, so I can consider it done.
:)