Add optional storages #35
Conversation
| @@ -167,8 +167,8 @@ function OAuthProvider() { | |||
| }; | |||
|
|
|||
| return $http.post(`${config.baseUrl}${config.grantPath}`, data, options).then((response) => { | |||
|
|
|||
|
Hi @jarbitlira! Looks great. It would be great if you could create a separate service to handle Also, I've added some comments... Mostly, code standards - try to maintain the code standards already in place. Thanks :) |
|
Hi @jarbitlira could you add tests please? |
| case 'cookies': | ||
| return ipCookie(config.name); | ||
| case 'localstorage': | ||
| return JSON.parse(localStorage.getItem(config.name)); |
There was a problem hiding this comment.
Use angular.fromJson instead of JSON.parse.
|
Please rename the PR from Feature/optional storages to Add optional storages. |
|
Hi everyone, thanks for yours comments, it's my first time with ES6. can you give me a guideline about code standards that are you using? I go to enhance the code, but first could you tell me, what do you think about this feature? |
|
@jarbitlira The feature is good. We should consider to switch ivpusic/angular-cookie by grevory/angular-local-storage which support multiple storage types, but still misses cookie secure flag option. Meanwhile, we can add this functionality with your PR. Regarding the code standards... We don't have them written anywhere. So, you'll need to take a look at the code and try to figure up the best you can. Sorry about that. |
jarbitlira
force-pushed
the
feature/optional-storages
branch
from
31d1a94 to
df5abac
Compare
jarbitlira
force-pushed
the
feature/optional-storages
branch
3 times, most recently
from
bd8d16e to
4cda40c
Compare
jarbitlira
force-pushed
the
feature/optional-storages
branch
from
4cda40c to
9ad4491
Compare
jarbitlira
force-pushed
the
feature/optional-storages
branch
from
756f225 to
2a9404d
Compare
|
+1 for angular-local-storage |
|
I think it hasn't been merged yet because no tests have been added. |
|
+1 right now the auth response isn't being serialized to the $cookie store ... |
|
I've gone ahead and made a separate pull request that replaces ngCookies with sessionStorage. sessionStorage works basically everywhere so we don't need to fall back to cookies. As for choosing between session and local storage we can tackle at a later point in time. |
I add this feature because if I use cordova/phonegab, cookies aren't detected and this plugin doesn't work correctly. For this reason I add the optional storage to localStorage and sessionStorage