Skip to content
This repository has been archived by the owner on Sep 15, 2022. It is now read-only.

Support for sessionStorage? #19

Closed
4kochi opened this issue Feb 23, 2015 · 20 comments
Closed

Support for sessionStorage? #19

4kochi opened this issue Feb 23, 2015 · 20 comments

Comments

@4kochi
Copy link
Contributor

4kochi commented Feb 23, 2015

Hi auth0 team,

great work so far. Are there any plans to also support sessionStorage as a store. Ideally the user can configure which store to use, but the fallback is always the sessionStorage. I am open to add a pull request with the feature.

Greetings

@mgonto
Copy link
Contributor

mgonto commented Feb 23, 2015

Hey there,

Why would you use sessionStorage instead of cookies as the fallback? Cookies are available everywhere!

I'm open to being able to set which store to use and implementing sessionStorage but still have cookies as fallback.

Makes sense?

Cheers!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 23, 2015

We want to store some application lifecycle data like paging and sorting options or form data. We use the session store because it's user specific and is persistent as long the user has a valid session. If we would use local storage, we have to manage which data is from what user since the data is persisted longer. So that would mean more management overhead. So the idea was that this module can also take care of the session storage.

Salute

@mgonto
Copy link
Contributor

mgonto commented Feb 23, 2015

Makes complete sense.

My thoughts are the following:

Let's make $cookies, localStorage and sessionStorage different services objects. Then, when you create a namespaced storage, you just pass the store back you want to use which is just the service name that'll be injected using $injector.

angular.module('app', ['angular-storage'])
.factory('Auth0Store', function(store) {
  return store.getNamespacedStore('auth0', 'sessionStorage');
})

What do you think about this aproach?

@mgonto
Copy link
Contributor

mgonto commented Feb 23, 2015

Are you willing to PR something like that?

@4kochi
Copy link
Contributor Author

4kochi commented Feb 23, 2015

Yeah, that sounds great. And the fallback pipeline would be local storage -> session storage -> cookie storage in case no store is passed by the user? I could try to implement this.

@mgonto
Copy link
Contributor

mgonto commented Feb 23, 2015

sounds good :). If you want, once you start doing commits or have the interface just shoot me a message and I can check the code :).

Thanks!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 23, 2015

I will start tomorrow. Btw, nice to see that you are using our karma-mocha-reporter. Makes me happy 😃

@mgonto
Copy link
Contributor

mgonto commented Feb 23, 2015

hahaha it works really nice :) I really like it!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 24, 2015

I guess i am ready. You can have a look at the changes in my fork (https://github.com/4kochi/angular-storage). I added a method setStorage() to the InternalStorage service to set a storage. The default storage is still the localStorage. The fallback for both localStorage and sessionStorage is the cookieStorage.
Theoretically now the storage can be replaced with any storage, as long as the storage service has the methods get(), set() and remove().

I also fixed some minor problems with tests and bower/package.json.

I hope you are happy with the changes...

@mgonto
Copy link
Contributor

mgonto commented Feb 24, 2015

Hey,

Thanks for the update. It looks good.

I only have one comment. I'd make the Internal store constructor and the getNamspacedStore methods actually receive as a parameter name, storageName, delimiter and make storageName default to localStorage.

That way, you can configure the storage when creating the store and not only afterwards.

Makes sense?

I'll update the README accordingly once the PR gets merged.

Thanks!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 24, 2015

Well, that would be a breaking change. And how to pass the param to the InternalStorage constructor? Because as user you inject the store service and receive an instance of the InternalStorage service. So no chance to pass a storageName. Or did I miss something?

@mgonto
Copy link
Contributor

mgonto commented Feb 24, 2015

Well, that would be a breaking change. And how to pass the param to the InternalStorage constructor? Because as user you inject the store service and receive an instance of the InternalStorage service. So no chance to pass a storageName. Or did I miss something?

Yes, that'd be a breaking change but I think it's worth it.

You can just pass the name and then use $injector.$get to get the service based on the name only like we're doing in the cookieStorage.

Makes sense?

@4kochi
Copy link
Contributor Author

4kochi commented Feb 24, 2015

Ok, but i am still struggling how the user can pass the storageName?

angular.module('app', ['angular-storage'])
.controller('Controller', function(store) {
  var myObj = {
    name: 'mgonto'
  };

  // store is already initialized, how to pass storageName?
  store.set('obj', myObj);
});

@mgonto
Copy link
Contributor

mgonto commented Feb 24, 2015

So what I'd do is 2 things.

First, for creating a new namespaced storage.

.factory('Auth0Store', function(store) {
  return store.getNamespacedStore('auth0', 'sessionStorage');
})

For changing the basic one, I'd convert the store into a provider and add the config method so that we can do

function config(storeProvider) {
  storeProvider.setStore('localStorage');
}

What do you think :)?

Sorry I wasn't clear before!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 24, 2015

Ok, that i was thinking about. But i tried to avoid it because it's a breaking change. But since this is no problem for you, i will convert the InternalStorage to a provider.

Maybe it makes sense that the function store.getNamespacedStore() uses an object as param. This would be more flexible. Otherwise if you want to get a namespaced store with delimiter but the default storage, you would call store.getNamespacedStore('auth0', null, '.')

store.getNamespacedStore({delimiter: '.', namespace: 'auth0'}) looks nicer. But thats just my personal opinion.

@4kochi
Copy link
Contributor Author

4kochi commented Feb 25, 2015

Changed store to a provider. Have a look.

@mgonto
Copy link
Contributor

mgonto commented Feb 25, 2015

Looks great :).

If tests pass just shoot the PR :). Have you added tests for sessionStorage?

Thanks!

@4kochi
Copy link
Contributor Author

4kochi commented Feb 25, 2015

Yes, i have. Will merge this evening. I can also add a code coverage report for the test so one can find all the code which is not covered by the tests.

@mgonto
Copy link
Contributor

mgonto commented Feb 25, 2015

Awesome thanks for all the help @4kochi

@4kochi
Copy link
Contributor Author

4kochi commented Feb 25, 2015

Your welcome 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants