Conversation
| private map = new Map<TKey, Promise<TResponse>>(); | ||
| private nonBlockingMap = new Map<TKey, TResponse>(); | ||
| private cache; | ||
| private nonBlockingCache; |
There was a problem hiding this comment.
Can you strongly define the types of these variables here?
| private map = new Map<TKey, Promise<TResponse>>(); | ||
| private nonBlockingMap = new Map<TKey, TResponse>(); | ||
| private cache; | ||
| private nonBlockingCache; |
There was a problem hiding this comment.
Also, I think nonBlockingMap must keep being a simple Map. The idea behind it is to always remember the last value of the given key, no matter if it has expired or not, so I must not be influenced by eviction rules, so I'd recommend to keep this property as it was
| this.cache.delete(key); | ||
| } else if (result !== Empty) { | ||
| this.pacer?.schedulePurge(key, ttl, result); | ||
| } |
There was a problem hiding this comment.
Looking at all the changes you've made here, you can remove almost all of them by keeping the current type and name of the variables, and also preventing breaking changes in a more rigid manner
| private removeImmediately: boolean; | ||
| private onReused?: (...args: any[]) => void; | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Does it make sense to throw an error in the constructor if someone tries to use nonBlocking = true and an eviction rule in the configs? It looks to me like exclusive concepts that should never be used together
| @@ -0,0 +1,5 @@ | |||
| export interface Cache<TResponse = unknown, TKey = string> { | |||
There was a problem hiding this comment.
Maybe it's better to keep the idiomatic around the Map word, instead of cache, to prevent confusion with dedicated cache services.
You can call it, for example, EvictionableMap. I also recommend you to invert the class generic parameters, to make it's contract compatible with Map<TKey, TResponse | Promise>
| @@ -0,0 +1,26 @@ | |||
| import { Cache } from '../core/cache'; | |||
|
|
|||
| export class SimpleCache<TResponse = unknown, TKey = string> | |||
There was a problem hiding this comment.
This class can be completely avoided if you invert the interface generic parameters, as you can just use Map instead
Here's a comprehensive pull request message describing the new changes:
🚀 Add Configurable Cache Strategies with Eviction Policies
📋 Summary
This PR introduces a major enhancement to the Remembered library by adding configurable cache strategies with eviction policies, allowing developers to manage memory usage more effectively and choose the most appropriate caching strategy for their use cases.
✨ New Features
🎯 Cache Eviction Policies
⚙️ Enhanced Configuration
🔧 Cache Factory
createCachefactory function that instantiates appropriate cache implementation based on configuration🏗️ Architecture Changes
Refactored Cache Structure
BaseCacheto dedicatedLinkedListclassCache Implementations
🧪 Testing Enhancements
Comprehensive Test Coverage
createCachefunctionTest Infrastructure
📚 Documentation Updates
Enhanced README
Updated TypeDoc
Breaking Changes
Use Cases
LRU Cache
Best for: Frequently accessed data, user sessions, general purpose caching
MRU Cache
Best for: Data that becomes stale quickly, temporary data, rate limiting
FIFO Cache
Best for: Time-sensitive data, logs, time-series data
🚀 Performance Benefits
Files Changed
src/cache-strategy/,src/remembered-config.ts,src/remembered.tstest/unit/cache-strategy/,test/unit/index.spec.tsREADME.md,docs/classes/remembered.mdpackage.json(test scripts)🔍 Testing
All tests pass with comprehensive coverage of new functionality and existing features.
Ready for review! 🎉