Allow true dynamic storage type on transaction

[benhoIIand]

As per my comment in #320, I don't think that the solution quite cut the mustard as it would permanently change the storage type for all further transactions.

My addition to the solution is to restore the previous storage type after a transaction.

The use case that is currently failing is:

// Assume the default storage is localStorage

// Sets the status to be true in sessionStorage
localStorageService.set('status', true, 'sessionStorage');

// Sets the status to be false in sessionStorage
// I would expect this to inherit the default storage type
localStorageService.set('status', false);

[c1moore]

I agree that is approach is superior to the one currently used. The current approach of permanently changing the type of storage used can result in unexpected behavior and even worse behavior that differs between uses of an app. For example, suppose we make 2 hypothetical async calls to the server as below

$http.get('/user').then(function(serverRes) {
  localStorageService.set('user', serverRes.data, 'sessionStorage');
});

$http.get('/route/that/returns/persistent/data').then(function(serverRes) {
  localStorageService.set('persistentData', serverRes.data);
});

If the second request resolves first, all is good. For a lack of better metrics, let's say this happens 50% of the time. The other 50% results in persistent data being stored in session storage instead. How might we know in which storage the persistent data was stored? What if we want to set a different variable with the same name in session storage that is independent of the one we set above? So many questions, no good answers.

My use case is slightly different than the one detailed above. 99.9% of the time I want to use localStorage to save data. On very rare occasions, though, it would be useful to store data that only lasts a single session. On these rare occasions, the logic related to these variables are localized in "session managers". These session managers are the only ones that even know session storage exists and I want it to remain this way. However, something horribly wrong happens if I try to use localStorageService to temporarily use session storage: it's not temporary. This leaves me with a horrible dilemma:

• Continue using localStorageService and set some arbitrary data that specifically uses local storage in the session managers to use localStorageService to use local storage in the future. This is a hacky solution and makes me feel like I kicked innocent puppies.
• Ignore the problem and hope things work. This feels worse than kicking innocent puppies.
• Stop using localStorageService

Since I neither like kicking innocent puppies nor ignoring bugs, the latter seems more appropriate to me.

[c1moore]

Another problem I forgot to mention is what if I eventually change the default default storage type? Now my session managers are changing the storage type to local storage unnecessarily. The current solution is just dirty and results in bad code.

If you don't like this solution because it might break existing code, you can add a 4th parameter that specifies if the change should be permanent (default) or not. Alternatively, you could create a new function that has the behavior described here (maybe name it something like setWithStorageType(key: String, value: Object, storageType: String)). I wouldn't say either solution is optimal, but they would be better than the current set up.

[skywalkerjx]

I agree with the solution that @c1moore has proposed.

[grevory]

Sorry for the delay on responding to this PR.

What happens when I call localStorageService.set('noType', 'useDefault'); or localStorageService.get('noType');? Looks like it does not default to local storage. Any reason for that?

[grevory]

Can someone add a default value? I would like to merge this PR in. Thank you.

[c1moore]

I can try to look into this shortly if @HollandBen is not able to fix this. On 05/17/2017 12:56 AM, Gregory Pike wrote: Can someone add a default value? I would like to merge this PR in. Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#344 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGDk75YyUZDskAMgI2Ao1LvrS9DcNj2sks5r6n4ZgaJpZM4L1D2F>.

[c1moore]

@grevory I don't understand what the problem is. Nothing related to the default storage type was changed nor how the storage type is selected. Basically, existing code was just wrapped with a try ... finally clause where the finally just reset the original value. I also tested to make sure that the default is originally used with code like below:

  localStorageService.set('key', 'value');

  console.log(localStorage['ls.key']);  // Prints: "value"
  console.log(localStorageService.get('key'));  // Prints: value

Here is a jsfiddle if you want to try yourself. Let me know if I'm misunderstanding what the problem is. Maybe a jsfiddle would be helpful.

That being said, I am going to create a PR since I found a few things that this original PR missed (such as localStorageService.length() and localStorageService.clearAll()).

[grevory]

Thanks @HollandBen.

Looking to update the docs for this change. Can someone take a look?

[grevory]

@c1moore There was nothing wrong. I overlooked the setStorageType function that considers a default. Lazy code reviewing is all.

[c1moore]

@grevory No problem, happens. Glad everything got merged in relatively quickly.

As for the docs, I don't know when I'll have the time, but if I get a few extra minutes I'll look at them.