refactor(sdk): Use Mapping instead of CachingMap
#2903
Conversation
teogeb
changed the title
Mapping instead of CacheMapMapping instead of CachingMap
Added `maxSize` and `maxAge` options to the `Mapping` class. These are needed in follow-up PR (#2903). Added two builder functions: `createCacheMap()` and `createLazyMap()`. This way the user of the class needs to decide if the Mapping if is a cache or just a lazy loading map. For caches it is required to configure `maxSize`. ## Type safety The constructor of `Mapping` is annotated as `@internal` so that in encourages the usage of the builder functions. If we make it private, the builder functions needs to have access to it somehow. Could e.g. be static methods. Or we could export `Mapping` as a type. ## Testing In unit tests we test the basic functionality for the lazy map, and use cache map only test the size limitations. This is maybe ok as in the `Mapping` the differences are abstracted out to the `delegate` (which is either a LRU cache or plain `Map`). ## Future improvements - Use `Mapping` instead of `CacheMap` in `StreamRegistry`/`StreamStorageRegistry` and remove the `CacheMap` class
There was a problem hiding this comment.
Looks good to me. I have a couple of loose suggestions:
I'd see the key to be a primitive by default OR something more complex if needed.
For a tuple
const a = new Mapping<[number, string], string>()
And for a string
const a = new Mapping()
And for a number
const a = new Mapping<number>()
So, in short, basic KeyType could be anything that satisfies K extends string | number | symbol. Then the applied key could be of type K | K[].
type Key<K> = K | K[]
class Mapping<K extends Key<string | number | symbol> = string, V = string> {}To simplify usage of more complex keys we could rewrite formLookupKey to JSON.stringify(key).
↑, maybe worth considering. It'd also allow to bypass 1-element tuple keys.
Added this functionality 👍 Didn't set any default types as |
Use
Mappingcache instead ofCachingMapinStreamRegistryandStreamStorageRegistry. After these modifications there are no usages forCacheMapinsdk, and we can remove it (in a follow-up PR).Testing
The implementations of
MappingandCachingMapare very different, but the functionality should be almost/exactly same. Double-check that there are no corner case in which behavior is significantly different. Checked that CachingMap tests pass also with the new implementation (see temporary commit e5bfdcf). The same functionality was already covered byMapping.test.ts, so there was no need keep anyCachingMap.test.tstest cases for theMappingclass.Other changes
Mapping#get()andMapping#valueFactory(), and return type ofMapping#values()Future improvements
CachingMap