feat: drop node4 support #39
Conversation
| return cached; | ||
| }; | ||
|
|
||
| cached.knownCaches = function knownCaches() { | ||
| return Object.keys(namedCaches); | ||
| return [...namedCaches.keys()]; |
There was a problem hiding this comment.
Q: should we return a MapIterator instead ?
There was a problem hiding this comment.
We wanted to keep the interface slim. Exposing a map iterator would lock us into using a Map internally in future versions as well. I think using an array here is great (a Set would also work but might be unnecessary overhead).
aaarichter
force-pushed
the
delay-writing
branch
from
b14727f to
0a518e9
Compare
|
@jkrems regarding the idea on "not waiting for the cache writing promise", do you think https://gist.github.com/aaarichter/22cbcdf636536d542d7e0e10c0d75064 would do? |
There was a problem hiding this comment.
Very nice! Thanks for working on this. I left a few minor comments. Also, I think it would be fair to drop "breaking change". We have treated dropping unsupported node versions as "usual maintenance" in the past. Nobody should be running those in production anyhow.
| let loadingNewValue = cache.staleOrPending[key] !== undefined; | ||
|
|
||
| if ((!hit || expired) && !loadingNewValue) { | ||
| if ( |
There was a problem hiding this comment.
Any reason why we went back to a huge if condition here?
There was a problem hiding this comment.
so the reason for the change is the likelihood of each part on the condition as well as calling functions / getting values
- cache.staleOrPending is set -> skip the more complex condition
&& (hit || expired) - hit: if no hit then
expireddoes not need to be calculated util.isExpired(wrappedValue && wrappedValue.b)don't check the value if any of the other cheaper checks pass
| return cached; | ||
| }; | ||
|
|
||
| cached.knownCaches = function knownCaches() { | ||
| return Object.keys(namedCaches); | ||
| return [...namedCaches.keys()]; |
There was a problem hiding this comment.
We wanted to keep the interface slim. Exposing a map iterator would lock us into using a Map internally in future versions as well. I think using an array here is great (a Set would also work but might be unnecessary overhead).
Changes: - [x] drop node4 support - [x] use Map API instead of object hash maps - [x] remove lodash dependency
aaarichter
force-pushed
the
delay-writing
branch
from
0a518e9 to
23dcc6a
Compare
|
update the commit messages and addressed (hopefully) the feedback for the noop and constant function |
| const Memcached = require('memcached'); | ||
|
|
||
| const noop = () => undefined; |
There was a problem hiding this comment.
Side note: Using function would have been shorter and wouldn't have closed over the weird module-scope this.
function noop() {}
const noop = () => undefined;| const getName = util.deprecate(function getName(name) { | ||
| return name || 'default'; | ||
| }, 'Unnamed caches and caches with non-string names are deprecated.'); | ||
| const getName = util.deprecate( |
There was a problem hiding this comment.
nit: This turns a previously named function into something that won't have a meaningful .name anymore which is a little worse in terms of debugging.
| @@ -1,6 +1,6 @@ | |||
| import assert from 'assertive'; | |||
| import Bluebird from 'bluebird'; | |||
There was a problem hiding this comment.
Huh, we should undo this eventually. Completely forgot that we were using babel script modules in this project. Good reminder. :)
Changes: