3 ms·
This comment is a good example of why "oh this is easy, I can write this myself" should not be the default approach in some cases. There's a severe bug in your
by Etheryte 4y ago
This comment is a good example of why "oh this is easy, I can write this myself" should not be the default approach in some cases. There's a severe bug in your set method, expiry times are not cleared when overwriting an expiring value with a value with no expiry. The library linked as the main thread does not have this bug.
- chrismorgan 4y agoTrue, that bug exists in what I wrote. Insert an `else { expiryTimes.delete(expiry); }` at the end of set to fix it. However, I note that KV.js has similarly severe bugs (some noted in my other comment in this thread—methods that use expired values, get() mangling falsy values) and its greater complexity makes it more likely to have bugs. Very often I agree with you about the dangers of writing it yourself. But very often I also sufficiently dislike how libraries are implemented, with unnecessary bloat like excessive flexibility (all the options and such) or explicit runtime type checking rather than just letting things go wonky or blow up if given bad data. In reality, in a case like this I would actually recommend something other than a generic container. Non-expiring keys? Use variables if the keys are a finite set of known values, or a Map if the keys are in fact arbitrary. Expiring keys? Use something vaguely like this, but probably wrapping Map<K, [number, V]>, made simpler by not supporting non-expiring keys (which mixture was what caused my bug, and which mixture I was never fond of).