refactor: rewrite, use memcached-elasticache & drop Node 8 #48

aaarichter · 2020-04-13T20:30:10Z

BREAKING CHANGES:

General

Dropped Node 8.x. Use Node 10.13 or higher.
Uses memcached-elasticache with AWS support instead of memcached. See https://github.com/jkehres/memcached-elasticache#readme
API functions no longer support callbacks.
API functions use native Promises instead of Bluebird.
cached() needs to be called with a string argument as name. If the name argument is undefined, it will fallback to "default".

`Cache`

Cache.setBackend() no longer returns the created backend.
Cache.getWrapped() is removed.
Cache backend APIs need to be promise based.
Cache private function have been renamed.
The following function are now private:
- Cache.applyPrefix
- Cache.end
- Cache.prepareOptions
- Cache.setBackend

`cache`

cache.set(), cache.get(), cache.getOrElse(), cache.unset() no longer support the callback argument. Use them as promise.
cache.set() no longer returns a value.
cache.set(key, value) accepts functions and promises as value.

Backends

Any client passed with the memcached backend options has to be an instance of Memcached.
Backend setters no longer return a value.
backend addType() does not return backend class anymore.

aaarichter · 2020-07-03T10:45:25Z

Hey @jkrems, @dbushong & @aotarola
would you be so kind and have a look at this PR 😼 . Thanks

dbushong · 2020-07-04T18:22:44Z

README.md

@@ -2,7 +2,7 @@

 A simple caching library, inspired by the [Play cache API](http://www.playframework.com/documentation/2.2.x/ScalaCache) and biased towards [showing stale data instead of dog piling](http://highscalability.com/strategy-break-memcache-dog-pile).
 The interface only exposes very limited functionality, there's no multi-get or deletion of cached data.
-The library is designed to support different caching backends, though right now only memcached is implemented.
+The library support different caching backends, though right now only memcached is implemented.


I think the original is more grammatically and semantically correct

dbushong · 2020-07-04T18:26:29Z

README.md

-  It's **highly** recommended to set a timeout.
-  If `timeout` is left `undefined`, no timeout will be set and the operations will only fail once the underlying client, e.g. [`memcached`](https://github.com/3rd-Eden/memcached), gave up.
+  Otherwise, there will be situations where one of the cache hosts goes down and reads hang for minutes while the memcached client retries to establish a connection.
+  It's **highly** recommended setting a timeout.


Former is more grammatically correct

dbushong · 2020-07-04T18:27:46Z

lib/backend.js

@@ -32,35 +32,53 @@

 'use strict';

-const typeMap = new Map();
+/**
+ * @typedef {Object} backend


style: it's easier to read the later annotations if types are capitalized, i.e. Backend not backend

or maybe BackendImpl if Backend would collide with the variable named that

dbushong · 2020-07-04T18:31:39Z

lib/backend.js

@@ -32,35 +32,53 @@

 'use strict';

-const typeMap = new Map();


General principle: particularly in an otherwise large PR, the bar for renaming not-obviously-wrong variables and making similar "optional" changes should be pretty high - they make the cognitive load for reviewers much higher, as they have to analyze each changed line to see how it's relevant to the fix/refactor in question.

i.e.: not saying that the new variable name isn't better, just that it wasn't necessarily an important change to make at this time.

dbushong · 2020-07-04T18:33:29Z

lib/backends/memcached.js

+ * @param {any} value
+ * @return {null|any}


Suggested change

* @param {any} value

* @return {null|any}

* @template T

* @param {T} value

* @return {null|T}

I think that's right (but double-check it)

dbushong · 2020-07-04T18:35:44Z

lib/backends/memcached.js

+    this._get = promisify(this.client.get).bind(this.client);
+    this._set = promisify(this.client.set).bind(this.client);
+    this._del = promisify(this.client.del).bind(this.client);


should these be instead: promisify(this.client.get.bind(this.client)) ?

dbushong · 2020-07-04T18:37:55Z

lib/backends/memcached.js

+   * @return {Promise<void>}
+   */
+  async set(key, value, options) {
+    await this._set(key, value, options.expire);


is it ok that this no longer returns the original value?

dbushong · 2020-07-05T16:11:42Z

If you're making breaking API changes, is there any reason to continue to support callback syntax?

Also, for the things that now return promises, you should review the README to make sure it's clear about callback use.

dbushong · 2020-07-05T16:20:09Z

lib/util.js

@@ -32,38 +32,77 @@

 'use strict';

-const Bluebird = require('bluebird');
+const timedOutError = new Error('operation timed out');


the stack trace is constructed at the point when you construct a new Error, so you want to do it when the error happens, and not before; e.g.:

// foo.js - BAD! const e = new Error('boom'); function a() { b(); } function b() { c(); } function c() { throw e; } a();

$ node foo.js foo.js:5 throw e; ^ Error: boom at Object.<anonymous> (/Users/dbushong/foo.js:1:11) at Module._compile (internal/modules/cjs/loader.js:778:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10) at Module.load (internal/modules/cjs/loader.js:653:32) at tryModuleLoad (internal/modules/cjs/loader.js:593:12) at Function.Module._load (internal/modules/cjs/loader.js:585:3) at Function.Module.runMain (internal/modules/cjs/loader.js:831:12) at startup (internal/bootstrap/node.js:283:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)

thanks for pointing this out.

dbushong · 2020-07-05T16:21:51Z

package.json

@@ -14,18 +14,23 @@
  },
  "scripts": {
    "lint": "npm-run-all lint:*",
-    "lint:js": "eslint .",
+    "lint:js": "eslint --fix .",


Careful with this - it means if you don't have a lintfix-on-save trigger set up in your editor, and you have un-prettier-ified changes, then when you run npm t it will rewrite your files out from under you, which may or may not be something safe or expected for a give developer

dbushong · 2020-07-05T16:22:15Z

package.json

-    "bluebird": "^3.3.3",
-    "memcached": "^2.2.2"
+    "@types/memcached": "^2.2.6",
+    "@types/node": "^13.11.1",


these belong in devDeps, i believe

dbushong · 2020-07-05T16:25:25Z

test/_helper.js

+
+exports.delay = function delay(ms) {
+  return new Promise(resolve => setTimeout(resolve, ms));
+};


as of node 8, i believe, this is the same as:

const { promisify } = require('util'); exports.delay = promisify(setTimeout);

dbushong · 2020-07-05T16:27:38Z

OK, I'm thru with a quick once-over.

aaarichter · 2021-05-05T10:24:09Z

If you're making breaking API changes, is there any reason to continue to support callback syntax?

yes you are right, it would make sense to further simplify the code by dropping callback syntax

aaarichter · 2021-05-06T20:36:05Z

Hey @dbushong @aotarola

sorry it took so long to address your feedback. Would you do another review? 🙇‍♂️

I rebased and updated the PR.

aotarola · 2021-05-06T21:54:11Z

README.md

+  // Set a key using a value)
+  await kittens.set('my.key', 'kitty');


Suggested change

// Set a key using a value)

await kittens.set('my.key', 'kitty');

Seems like a dup from line 28 ?

oh I didn't see. good spot

aotarola · 2021-05-06T21:58:18Z

README.md

+  const data2 = await kittens.get('my.key').catch(err => {
+      throw err;
+    }


Suggested change

const data2 = await kittens.get('my.key').catch(err => {

throw err;

}

try {

const data2 = await kittens.get('my.key')

}catch(err) {

// log it somewhere ...

}

How about using try/catch block here? just to be consistent with the synchronous style of async/await ?

dbushong · 2021-05-07T15:25:17Z

lib/backend.js

-addType('noop', require('./backends/noop'));
+/**
+ * @param {string} type
+ * @param {Class} Backend


should this be {BackendImpl} ?

dbushong

Skimmed; lots of changes; seems ok?

aotarola

💚

- [x] chore: drop Node 8 support. Min required version is [email protected] - [x] refactor(deps): remove `lodash` and `bluebird` - [x] refactor(cache): rewrite `Cache` with ES6 class syntax - [x] refactor: `cache.set` no longer returns a value - [x] refactor: make inofficial cache api private - [x] fix: handle functions or promises as `cache.set` values as described in README.md - [x] fix: remove deprecation message, that warned when backend `name` was not a string. It will throw a type error now. - [x] deps: use `memcached-elasticache` instead of `memcached` - [x] chore(deps): upgrade dependencies - [x] test: add additional test to increase test coverage - [x] docs(README): update API section BREAKING CHANGE: - Dropped Node 8.x. Use Node 10.13 or higher. - Uses `memcached-elasticache` with AWS support instead of `memcached`. See https://github.com/jkehres/memcached-elasticache#readme - API functions no longer support callbacks. - API functions use native Promises instead of `Bluebird`. - `cached()` needs to be called with a string argument as name. If the `name` argument is undefined, it will fallback to `"default"`. - `Cache.setBackend()` no longer returns the created backend. - `Cache.getWrapped()` is removed. - Cache backend APIs need to be promise based. - Cache private function have been renamed. - The following function are now private: - `Cache.applyPrefix` - `Cache.end` - `Cache.prepareOptions` - `Cache.setBackend` - `cache.set()`, `cache.get()`, `cache.getOrElse()`, `cache.unset()` no longer support the callback argument. Use them as promise. - `cache.set()` no longer returns a value. - `cache.set(key, value)` accepts functions and promises as value. - Any client passed with the memcached backend options has to be an instance of `Memcached`. - Backend setters no longer return a value. - backend `addType()` does not return backend class anymore.

aaarichter force-pushed the updates branch from 521e642 to a79e5b1 Compare April 13, 2020 21:09

aaarichter changed the title ~~refactor: no bluebird, no lodash~~ refactor: no bluebird, no lodash & code refactor Apr 14, 2020

aaarichter force-pushed the updates branch 2 times, most recently from 075d877 to 65404ac Compare July 3, 2020 10:20

aaarichter force-pushed the master branch from c05176b to aa75571 Compare July 3, 2020 10:30

aaarichter force-pushed the updates branch from 65404ac to 45096be Compare July 3, 2020 10:32

i-tier-bot added the semver-major label Jul 3, 2020

aaarichter force-pushed the updates branch from 45096be to 4f3a232 Compare July 3, 2020 10:38

aaarichter marked this pull request as ready for review July 3, 2020 10:42

aaarichter requested a review from aotarola July 3, 2020 10:43

dbushong reviewed Jul 4, 2020

View reviewed changes

dbushong reviewed Jul 5, 2020

View reviewed changes

aaarichter force-pushed the updates branch from 4f3a232 to e8157e4 Compare May 6, 2021 20:31

i-tier-bot added semver-patch and removed semver-major labels May 6, 2021

aaarichter changed the title ~~refactor: no bluebird, no lodash & code refactor~~ refactor: rewrite, use memcached-elasticache & drop Node 8 May 6, 2021

aaarichter force-pushed the updates branch from e8157e4 to 65dd518 Compare May 6, 2021 20:34

aaarichter requested a review from dbushong May 6, 2021 20:34

aaarichter force-pushed the updates branch from 65dd518 to 6812c8f Compare May 6, 2021 20:39

i-tier-bot added semver-major and removed semver-patch labels May 6, 2021

aotarola reviewed May 6, 2021

View reviewed changes

aaarichter requested a review from aotarola May 7, 2021 09:20

dbushong reviewed May 7, 2021

View reviewed changes

dbushong approved these changes May 7, 2021

View reviewed changes

aaarichter mentioned this pull request May 8, 2021

Update docs or change default backend from "noop" #37

Closed

aaarichter force-pushed the updates branch from 85eac84 to d7036f8 Compare May 10, 2021 09:28

aotarola approved these changes May 12, 2021

View reviewed changes

Andreas Richter added 5 commits June 4, 2021 16:31

fix: address feedback

a6da5db

refactor: set memory backend as default. Fixes #37

7f9bcf4

test: fix expire test

0ca947f

feat: add cache.flush()

e388cf8

aaarichter force-pushed the updates branch from 217ca0a to e388cf8 Compare June 4, 2021 14:33

chore(deps): update devDependencies

00a9163

aaarichter merged commit 1a7a256 into master Jun 27, 2021

aaarichter deleted the updates branch June 27, 2021 15:44

aaarichter mentioned this pull request Jul 16, 2021

add cache.flush() #57

Closed

- * @param {any} value
- * @return {null|any}
+ * @template T
+ * @param {T} value
+ * @return {null|T}

		// Set a key using a value)
		await kittens.set('my.key', 'kitty');

refactor: rewrite, use memcached-elasticache & drop Node 8 #48

refactor: rewrite, use memcached-elasticache & drop Node 8 #48

Uh oh!

Conversation

aaarichter commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BREAKING CHANGES:

General

Cache

cache

Backends

Uh oh!

aaarichter commented Jul 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbushong Jul 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbushong commented Jul 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbushong commented Jul 5, 2020

Uh oh!

aaarichter commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaarichter commented May 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbushong left a comment

Choose a reason for hiding this comment

Uh oh!

aotarola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaarichter commented Apr 13, 2020 •

edited

Loading

`Cache`

`cache`

dbushong Jul 4, 2020 •

edited

Loading

aaarichter commented May 5, 2021 •

edited

Loading