Various fixes #5
Conversation
| @@ -1,5 +1,8 @@ | |||
| # Change Log | |||
|
|
|||
| - [#5] Allow for call-level age overrides as specified in the README. | |||
| - [#4] Bump various dev-dependencies | |||
There was a problem hiding this comment.
Doesn't this PR make #4 useless?
There was a problem hiding this comment.
sure, unless we want them to be separate and more targeted in scope.
There was a problem hiding this comment.
Did the merge separately.
lib/multi-level.js
Outdated
| }; | ||
|
|
||
| if (shouldCache && typeof shouldCache !== 'function') { | ||
| const { shouldCache = this.shouldCache } = opts; |
There was a problem hiding this comment.
Just to be stylistically consistent, just use getValue again
const shouldCache = getValue(opts, 'shouldCache', this.shouldCache);
lib/multi-level.js
Outdated
| * @param {ShouldCache} [opts.shouldCache=()=>true] - Function to determine whether to not we should cache the result | ||
| * @param {Cache[]} opts.caches - An Array of cache objects. See `./fs` and `./memory` for sample caches. | ||
| * @param {Object} opts Options for the Client instance | ||
| * @param {Number} [opts.maxAge=600,000] The duration, in milliseconds, before a cached item expires |
There was a problem hiding this comment.
The comma should probably be removed here, as it isn't in the actual default. (We might want to include a version of the default with the comma in the description for readability).
lib/multi-level.js
Outdated
| * @returns {Any} The value of the property | ||
| */ | ||
| function getValue(opts, key, defaultValue) { | ||
| if (key in opts) { |
There was a problem hiding this comment.
Not a must, but could be condensed:
return (key in opts) ? opts[key] : defaultValue;| * | ||
| * @async | ||
| * @returns {Promise<GetResult>} a Promise which resolves to an object containing | ||
| * a `value` property and a `fromCache` boolean indicator. | ||
| */ | ||
| async get(key, opts, updateFn) { | ||
| opts = opts || {}; |
There was a problem hiding this comment.
Why not put the default on the argument?
async get(key, opts = {}, updateFn)There was a problem hiding this comment.
because it's an awkward API and requires you to do:
.get(myKey, undefined, someFn);since passing a null doesn't invoke the param default.
| * @param {String} [opts.maxAge] The duration in milliseconds before a cached item expires | ||
| * @param {Number} [opts.maxStaleness=0] The duration, in milliseconds, in which expired cache items are still served | ||
| * @param {ShouldCache} [opts.shouldCache] A function to determine whether or not we should cache the item | ||
| * @param {UpdateFn} updateFn async function that defines how to get a new value | ||
| * | ||
| * @async | ||
| * @returns {Promise<GetResult>} a Promise which resolves to an object containing | ||
| * a `value` property and a `fromCache` boolean indicator. | ||
| */ | ||
| async get(key, opts, updateFn) { |
There was a problem hiding this comment.
Maybe something like optional-options could be used to avoid having to pass in an empty object. 😉
There was a problem hiding this comment.
it's not a callback though, and while it has the same shape as what optional-options wants, its semantically different. Really the right thing to do is making a breaking change and reverse the order of the params. We should consider doing that at some point. Not sure its warranted just yet though. something to bear in mind (I'll open an issue)
Co-Authored-By: msluther <[email protected]>
…d-cache into fix-file-system-cache
…x-file-system-cache
(This was built on top of #4)
Fixes include:
diagnostics)maxAgeandmaxStalenessto be sent along with calls togetas implied by the unit tests and called out in the README.