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

Promises don't behave as expected #135

Closed royaldark closed 9 years ago

royaldark commented 9 years ago

Just upgraded from 3.0.0 and much to my surprise, lots of my caching stopped working. The first breaking version is 3.0.2 (despite being called a "backwards compatible" update). It seems this is due to a change in the handling of cached promises starting in 3.0.2 (and related to issues #115 and #116).

There is no mention in the docs that promises are treated differently than any other value by angular-cache, and in fact that behavior seems undesirable. Why are promises treated differently now? Could this at least by an option for each cache?

There may be an underlying philosophical question about whether angular-cache should always behave identically to $cacheFactory, or whether angular-cache has developed enough of an API of its own to diverge when $cacheFactory changes.

At least for my particular use case, I expect angular-cache to be a dumb storage API which treats all data equally. If that is not the case, it would be great if the documentation noted that, and preferably if there were an option to disable that behavior. It would also be nice if the changelog were updated to reflect the fact that 3.0.2 is not a backwards compatible update.

jmdobry commented 9 years ago

An option to allow storing promises directly in the cache was recently added to angular-cache 2.x, but has not yet been ported to 3.x.

Maintaining compatibility with $cacheFactory is difficult, as angular-cache has a lot more features than $cacheFactory. I think you'll agree that a promise cannot be stored in localStorage, a problem that $cacheFactory doesn't have.

You're welcome to submit a pull request (port #131 to 3.x perhaps), as I have limited time to donate to free, open-source projects like this.

jmendiara commented 9 years ago

After trying angular-cache as a direct replacement for an $http cache, I experienced the the same issues than @royaldark

Trying to port #131 to v3.0, I've found several issues:

  1. It returns undefined when putting promises, breaking the put API (that was inherited, I think)
  2. When updating a key that was previously a promise, it is not get deleted from the promiseStorage, causing memory leaks
  3. Does not track the amount of promises stored (potentially having Infinite)
  4. Promises don't make use of a cache bucket. Promises should use one cache bucket to maintain compatibility with default $cacheFactory caches implementation. (This angular approach is completely debatable, but it's efficient).

The 1st and 2nd issues can be resolved quickly. 3rd and 4th can be done with a great refactor, or document that as "features" if they are design decisions

tl;dr I think a promise stored in the cache should not react to its status. When you put a promise in the cache, it automatically inserts its value in the the cache once resolved. This breaks SoC, introducing coupling with the resolved value format from an $http promise, and stores the resolved value in localStorage several times with the same value when the same request is done in the same event loop iteration. I wouldn't recommend the default behaviour (once implemented) in the documentation.

jmdobry commented 9 years ago

@jmendiara Tell me how #141 works for you.

jmendiara commented 9 years ago

@jmdobry comments and code review in #141

royaldark commented 9 years ago

Thanks so much for this @jmdobry! I meant to look at it at some point, but I too have limited time for these projects :)