Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change maxCacheCount default? #57

Closed
stnor opened this issue Nov 28, 2019 · 4 comments · Fixed by #59
Closed

Change maxCacheCount default? #57

stnor opened this issue Nov 28, 2019 · 4 comments · Fixed by #59

Comments

@stnor
Copy link

stnor commented Nov 28, 2019

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
Copy link
Owner

@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
Copy link
Author

stnor commented Nov 28, 2019

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
Copy link
Owner

@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
Copy link
Author

stnor commented Nov 30, 2019

Yes, sounds great!

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

Successfully merging a pull request may close this issue.

2 participants