angelnikolov / ts-cacheable

Observable/Promise Cache Decorator
https://npmjs.com/package/ts-cacheable
ISC License
340 stars 42 forks source link

Change maxCacheCount default? #57

Closed stnor closed 4 years ago

stnor commented 4 years ago

Hi, Thanks for making the effort to provide a caching library. It looks very promising!

Some feedback I especially like the ability to evict the cache, but as #56 points out its not currently very useful for maxCacheCount. Also, "cacheBusting" is known as cacheEviction, and is a well-known term. Along the same lines, I don't think it makes sense to limit cache size based on the number of combinations of paramaters for a method. Perhaps it would be better to separate the Cache storage config and the cache strategy? I find the default behaviour a little bit odd compared to other caching libraries I have used eg eh-cache et.c Typically, a single Cacheable decorator will key up all parameters and maintain a cache for each combination, as compared to the default behavior for ngx-cachable of killing the cache when calling with a new parameter. Please consider changing default behavior for Cacheable. I find it counter-intuitive compared to other libraries/standards, and there are already a few issues that has arisen from this, like #26 and #42

Rather than specifying a maxCacheCount of Number.POSITIVE_INFINITY and a TTL on all the cache-decorators, it would be useful to override the defaults globally. Is that possible?

angelnikolov commented 4 years ago

@stnor Thanks for the feedback! So what you are suggesting is basically, keeping the cache for a parameter forever? Then, the only way to evict the cache would be the user to use a cacheBuster and evict it manually, which makes the cache layer a bit more imperative. Can you provide me with an example use case where you would need a global maxCacheCount? We can add that easily, in fact I thought of making the whole cacheConfig available as a global setting, since now we can only provide small portion of the config settings to the whole application.

Also #56 points out that we need a selective cache eviction. Like, you could want to only evict certain values from the cache, when you call a cache buster or the global one, while with the maxCacheCount you don't care about specific cached values, you just want to remove the least recent one. We can of course add an option like keepAlive (or another term if you prefer) which will basically tell the decorators to keep the cache no matter what, unless it's evicted manually. How does that sound? I wouldn't want to introduce it as a default behaviour because that will be a large breaking change.

stnor commented 4 years ago

I was thinking that as a user I would like to register defaults with a static method, so that if I want a certain TTL or maxCacheCount by default, I could decide so for myself, keeping things DRY.

angelnikolov commented 4 years ago

@stnor Yes, this is what I meant by the global config. It will be something like what we have now for global storage strategy, or:

GlobalCacheConfig.storageStrategy = DOMStorageStrategy;

To evolve that I am thinking of introducing all options of the cache config globally, like maxCacheCount, maxAge and slidingExpiration. Is that something that would help you? Also, we can introduce a global and local cache config option of keepAlive which will keep all caches indefinitely.

stnor commented 4 years ago

Yes, sounds great!