groupon / node-cached

A simple caching library for node.js, inspired by the Play cache API
BSD 3-Clause "New" or "Revised" License
94 stars 15 forks source link

Fix confusing conventions in memcache client docs #20

Open jvivs opened 8 years ago

jvivs commented 8 years ago

Cache vs cached is confusing, easily leading astray someone familiar with TitleCaseForClasses.

Skimming the docs, it is easy to assume Cache is an object constructor, not an instance of an object with a handle to a configured cache client.

jkrems commented 8 years ago

Hi! Thanks for bringing this up. Always happy to make the docs easier to understand. Would Cache::set(key, value, opts, cb) -> Promise[Value] be more obvious?

jvivs commented 8 years ago

I'd suggest avoiding an uppercase for Cache unless it is actually a class-like thing.

It appears that cached is a factory method for creating new instances of Cache, correct? If that's the case, cacheClient.set(key, value, opts, cb) -> Promise[Value] seems like it would communicate what's actually going on.

Happy to open a PR w/ a docs update if you'd like.

On Tue, Dec 8, 2015 at 7:00 PM, Jan Olaf Krems notifications@github.com wrote:

Hi! Thanks for bringing this up. Always happy to make the docs easier to understand. Would Cache::set(key, value, opts, cb) -> Promise[Value] be more obvious?

— Reply to this email directly or view it on GitHub https://github.com/groupon/node-cached/issues/20#issuecomment-163061490.

jkrems commented 8 years ago

But then there's a mismatch between -> Promise[Value]/-> Cache and cacheClient. E.g. there's no 1:1 mapping that can tell the reader that cacheClient is an instance of Cache (which it is).

P.S.: I made the suggestion of Cache::foo because :: is a somewhat common way to denote "prototype method foo of the constructor Cache".

jvivs commented 8 years ago

It seems the thing returned by cached() that you actually interact with is a layer of abstraction--sometimes its an in-memory cache and sometimes its a memcache pool running elsewhere.

The fact that cached can give you a unified api to both an in-memory Cache and to a memcache pool is really nice because you don't have to think about the particulars of managing cache with a given backend. But, when you're interacting with the object returned by cached, you aren't guaranteed to have a handle on the cache itself. Otherwise it seems like this should delete the cache contents when configured against a pool:

var myCache = cached({/* memcache pool configuration */}); delete myCache;

:grin:

edit: I have no clue why GFM is failing me in this comment. Apologies for the code blocks!

jkrems commented 8 years ago

Not sure I follow. Cache is the only interface that cached provides. It's always the same class, no matter what the backend is. You'll never get the actual memcached client. myCache in your example is always an instance of Cache.

I'm not sure what delete myCache is supposed to mean. If we ever add a myCache.flush(), then yes: I'd expect that to clear out the data from a memcached backend just like it would for an in-memory cache. Setting myCache = null will delete neither memcached nor in-memory data since cached will retain it.

But I'm not sure what the location of the cached data has to do with the return type docs..? Your initial point was "people could think Cache is a constructor". Which it is. Which is why I'm still a bit confused. My first guess was that you meant "Cache.get could be interpreted as a static method on the constructor". But maybe that's not what you're talking about?