hapijs / catbox-memory

Memory adapter for catbox
Other
33 stars 30 forks source link

fix: make class extendable #83

Closed hpyzik closed 3 years ago

hpyzik commented 3 years ago

Functions created via the class syntax are not callable. So there is no need to make the extra assertion that at the same time prevent from the class extending.

const CatboxMemory = require("@hapi/catbox-memory");
class InMemoryCacheStore extends CatboxMemory {}
const store = new InMemoryCacheStore(); // => Error: Memory cache client must be instantiated using new ...
hpyzik commented 3 years ago

Seems that tests are not stable. I amended this commit three times. First build had one failed check (ci / macos node@14), second build had three failed checks (ci / macos node@14, ci / ubuntu node@14, ci / ubuntu node@12), third build one failed check (ci / ubuntu node@* ). Always the same test fails 29) Memory set() removes multiple items from the cache when they expire:

hpyzik commented 3 years ago

I know that is not the best way to fix the test, some kind of waitFor check would be better, but I think it's still acceptable.

devinivy commented 3 years ago

Thanks!

Marsup commented 3 years ago

Wouldn't it have been better to change the assert to use instanceof instead of removing it? That would have worked with inheritance.

kanongil commented 3 years ago

Nah, it is redundant. Class constructors can only be called with new.

hpyzik commented 3 years ago

Thank you guys!