search

jmdobry / angular-cache

angular-cache is a very useful replacement for the Angular 1 $cacheFactory.
http://jmdobry.github.io/angular-cache
MIT License
1.39k stars 156 forks source link

https://github.com/jmdobry/angular-cache/issues/89 seems, fixed #90

Closed GulinSS closed 10 years ago

GulinSS commented 10 years ago

Test it:

  1. Open in two tabs: http://jsfiddle.net/EL2yf/1/
  2. Write to input a string in first tab
  3. Switch to second tab and press 'Refresh'. You should see your string.
coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling da29d19c9a1346efe1696375a2f0c1c0b92c7fbe on GulinSS:master into 8b7b43b9ce1aee0e9734cd15e77e55e77741fdb2 on jmdobry:master.

jmdobry commented 10 years ago

So I can review this and better understand what was broken, can you create a fiddle that shows the bug?

jmdobry commented 10 years ago

Okay, I've figured out what the "problem" is. The _verifyIntegrity method is one-directional–it forces localStorage to mirror the state of the data that's in memory. Looking at this fiddle (http://jsfiddle.net/4DRAZ/2/), clicking refresh on the second doesn't show the string on the first tab because the get operation on the second tab overwrites whatever is in localStorage with what is in memory in the second tab.

I think this is a "problem" when you have the app open in two tabs in the same browser. The verifyIntegrity functionality was intended to preserve localStorage, or prevent any other tab from messing with the data–not to synchronize data across tabs. The question is how to reconcile these two different behaviors/use cases.

I think the necessary fix is to modify the behavior of verifyIntegrity so instances of angular-cache can play nicely across multiple tabs. Right now (and this is how it was originally designed) each instance on each tab is trying to keep localStorage in sync with its own data and not allow "external" (like a cache in a different tab) to mess with the data.

I propose that verifyIntegrity be modified to be a two-way synchronization, instead of a one-way. This will fix the issue you raised in #89.

GulinSS commented 10 years ago

@jmdobry you can do it as you want. :-) I use a bit different approach just because to keep backward compatibility.

GulinSS commented 10 years ago

I found an sad error in my approach. I will fix it and resend the pull request.