Skip to content
This repository was archived by the owner on Apr 28, 2025. It is now read-only.

feat: drop node4 support #39

Merged
merged 2 commits into from
Sep 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"extends": "groupon/node4"
"extends": "groupon/node6"
}
8 changes: 4 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
language: node_js
node_js:
- 4.6.1
- 6.11.5
- 8.9.0
- v6.14.3
- v8.11.3
- v10.5.0
deploy:
- provider: script
script: ./node_modules/.bin/nlm release
skip_cleanup: true
'on':
branch: master
node: 8.9.0
node: 10.5.0
services: memcached
6 changes: 3 additions & 3 deletions lib/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

'use strict';

const typeMap = Object.create(null);
const typeMap = new Map();

function isBackend(object) {
return typeof object.get === 'function' && typeof object.set === 'function';
Expand All @@ -46,15 +46,15 @@ exports.create = function create(options) {
}

const type = options.type || 'noop';
const BackendClass = typeMap[type];
const BackendClass = typeMap.get(type);
if (!BackendClass) {
throw new Error(`${type} is not a supported cache backend type`);
}
return new BackendClass(options);
};

function addType(type, BackendClass) {
typeMap[type] = BackendClass;
typeMap.set(type, BackendClass);
return BackendClass;
}
exports.addType = addType;
Expand Down
8 changes: 5 additions & 3 deletions lib/backends/memcached.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@
'use strict';

const promisify = require('bluebird').promisify;
const _ = require('lodash');
const Memcached = require('memcached');

const noop = () => undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: Using function would have been shorter and wouldn't have closed over the weird module-scope this.

function noop() {}
const noop = () => undefined;


function createClient(options) {
if (options.client) {
return options.client;
Expand Down Expand Up @@ -63,11 +64,12 @@ MemcachedBackend.prototype.get = function get(key) {
};

MemcachedBackend.prototype.set = function set(key, value, options) {
return this._clientSet(key, value, options.expire).then(_.constant(value));
const constant = () => value;
return this._clientSet(key, value, options.expire).then(constant);
};

MemcachedBackend.prototype.unset = function unset(key) {
return this._clientDel(key).then(_.noop);
return this._clientDel(key).then(noop);
};

MemcachedBackend.prototype.end = function end() {
Expand Down
12 changes: 6 additions & 6 deletions lib/backends/memory.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,29 +38,29 @@ const util = require('../util');

/* Stores everything just in memory */
function MemoryBackend() {
this.cache = Object.create(null);
this.cache = new Map();
this.type = 'memory';
}
module.exports = MemoryBackend;

MemoryBackend.prototype.get = function get(key) {
let wrappedValue = this.cache[key] || null;
let wrappedValue = this.cache.get(key) || null;
if (util.isExpired(wrappedValue && wrappedValue.e)) {
wrappedValue = null;
delete this.cache[key];
this.cache.delete(key);
}
return Bluebird.resolve(wrappedValue ? wrappedValue.d : null);
};

MemoryBackend.prototype.set = function set(key, value, options) {
this.cache[key] = {
this.cache.set(key, {
d: value,
e: util.expiresAt(options.expire),
};
});
return Bluebird.resolve(value);
};

MemoryBackend.prototype.unset = function unset(key) {
delete this.cache[key];
this.cache.delete(key);
return Bluebird.resolve();
};
5 changes: 2 additions & 3 deletions lib/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@

'use strict';

const _ = require('lodash');

const Backend = require('./backend');
const getOrElse = require('./get-or-else');
const util = require('./util');
Expand Down Expand Up @@ -81,7 +79,7 @@ Cache.prototype.end = function end() {
};

Cache.prototype.prepareOptions = function prepareOptions(options) {
return _.extend({}, this.defaults, options);
return Object.assign({}, this.defaults, options);
};

Cache.prototype._set = function _set(key, val, options) {
Expand Down Expand Up @@ -119,6 +117,7 @@ Cache.prototype._applyTimeout = function _applyTimeout(value) {
Cache.prototype._getWrapped = function _getWrapped(key) {
return this._applyTimeout(this.backend.get(key));
};

// For backwards compatibility, eventually we should deprecate this.
// It *should* be a private API.
Cache.prototype.getWrapped = Cache.prototype._getWrapped;
Expand Down
35 changes: 19 additions & 16 deletions lib/cached.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,33 +33,36 @@
'use strict';

const Bluebird = require('bluebird');
const _ = require('lodash');
const util = require('util');

const Cache = require('./cache');

let namedCaches = Object.create(null);
const namedCaches = new Map();

const getName = util.deprecate(function getName(name) {
return name || 'default';
}, 'Unnamed caches and caches with non-string names are deprecated.');
const getName = util.deprecate(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

name => name || 'default',
'Unnamed caches and caches with non-string names are deprecated.'
);

function cached(name, options) {
if (typeof name !== 'string') {
name = getName(name);
}

if (!(name in namedCaches)) {
namedCaches[name] = cached.createCache(
_.extend(
{
name: name,
},
options || {}
if (!namedCaches.has(name)) {
namedCaches.set(
name,
cached.createCache(
Object.assign(
{
name: name,
},
options || {}
)
)
);
}
return namedCaches[name];
return namedCaches.get(name);
}
module.exports = cached;

Expand All @@ -68,17 +71,17 @@ cached.createCache = function createCache(options) {
};

cached.dropNamedCaches = function dropNamedCaches() {
namedCaches = Object.create(null);
namedCaches.clear();
return cached;
};

cached.dropNamedCache = function dropNamedCache(name) {
delete namedCaches[name];
namedCaches.delete(name);
return cached;
};

cached.knownCaches = function knownCaches() {
return Object.keys(namedCaches);
return [...namedCaches.keys()];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: should we return a MapIterator instead ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

};

cached.deferred = function deferred(fn) {
Expand Down
9 changes: 5 additions & 4 deletions lib/get-or-else.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
'use strict';

const Bluebird = require('bluebird');
const _ = require('lodash');

const util = require('./util');

Expand Down Expand Up @@ -77,10 +76,12 @@ function getOrElse(cache, key, val, opts) {

function verifyFreshness(wrappedValue) {
const hit = !!wrappedValue;
const expired = util.isExpired(wrappedValue && wrappedValue.b);
let loadingNewValue = cache.staleOrPending[key] !== undefined;

if ((!hit || expired) && !loadingNewValue) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we went back to a huge if condition here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 expired does not need to be calculated
  • util.isExpired(wrappedValue && wrappedValue.b) don't check the value if any of the other cheaper checks pass

!loadingNewValue &&
(!hit || util.isExpired(wrappedValue && wrappedValue.b))
) {
cache.staleOrPending[key] = refreshValue();
loadingNewValue = true;
}
Expand All @@ -96,7 +97,7 @@ function getOrElse(cache, key, val, opts) {

return cache
._getWrapped(key)
.catch(_.constant(null))
.catch(() => null)
.then(verifyFreshness);
}
module.exports = getOrElse;
7 changes: 3 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
},
"dependencies": {
"bluebird": "^3.3.3",
"lodash": "^4.6.1",
"memcached": "^2.1.0"
"memcached": "^2.2.2"
},
"devDependencies": {
"assertive": "^2.1.0",
Expand All @@ -41,8 +40,8 @@
"eslint-plugin-import": "^2.8.0",
"eslint-plugin-node": "^5.2.1",
"eslint-plugin-prettier": "^2.2.0",
"mocha": "^3.1.2",
"nlm": "^3.0.0",
"mocha": "^5.2.0",
"nlm": "^3.3.1",
"prettier": "^1.6.1"
},
"author": {
Expand Down
2 changes: 1 addition & 1 deletion test/timeout.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import assert from 'assertive';
import Bluebird from 'bluebird';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, we should undo this eventually. Completely forgot that we were using babel script modules in this project. Good reminder. :)

import { identity } from 'lodash';
const identity = val => val;

import Cache from '../lib/cache';

Expand Down