fix(lru): don't clear an already cached key on set

[jviide]

This pull request modifies how the LRU cache implementation handles setting a key that's already in the cache. Previously, if an already cached key was set again, then it was always cleared from the cache:

> const cache = new LRUCache();
> cache.set(1, 1);
> cache.set(1, 2);
> cache.get(1);
undefined

This pull request's modified version returns 2 for cache.get(1). The newly set key is still bumped to the last place in the LRU queue. There are two new tests to check this behavior.

The current semver implementation might never trigger this behavior, in which case this pull request is just future-proofing.

[wraithgar]

This is a great example of practically dead code that unit tests could never catch. The way that classes/range.js uses this lib is to call get and if it's in there return it. If it's not, it creates a new one and sets it. All of the code between creating the range and setting it is synchronous, no other event loop action could happen. It's currently impossible for a set to even be called on an entry that already exists.

Tests that only interacted with ranges would have shown that some of this code was unreachable from the surface area of the library. Unfortunately those tests don't exist, and unless someone wanted to put in that work we have what we have today.

With all that in mind, I propose that the real optimization here is to remove the first this.delete(key) altogether. It's a noop during the current actual use case of this lru code.

[wraithgar]

Suggested change

this.delete(key)

[jviide]

Removing the this.delete(key) would turn the LRU cache implementation into a mixture of LRU and FIFO caches. It may not be relevant with the module's current usage pattern. However, it could lead to a surprise down the road if the cache module's usage is expanded.

I suggest either keeping the delete call, removing the delete while calling the cache implementation something else than an LRU, or just closing this pull request as unnecessary (as it does not affect currently observable behavior like you pointed out). I ran the benchmarks and removing or keeping the delete call seems to make no difference there, though there are currently no benchmarks that heavily exercise the .set method.

[wraithgar]

Gonna err on the side of caution then and close this as not necessary. Thanks for the time spent, all the same.