Add `status` option for tracking cache behavior

[isaacs]

Fix: https://github.com/isaacs/node-lru-cache/issues/271

@yoshigev can you take a look and see what you think? What might be missing/confusing/etc? Better names for things? Suggestions welcome, this is just a first throw of paint at the wall.

You can install it by running:

npm install github:isaacs/node-lru-cache#isaacs/status-option

[github-actions[bot]]

size-limit report 📦

Path	Size
./index.js	4.99 KB (+12.86% 🔺)

[yoshigev]

Wow. Thanks @isaacs ! I did not expect such a quick response, and certainly not such a big change within several hours.

I'm sorry that I'm not familiar enough with the code itself to do a code review, but I did try it so I can comment on the API.

First of all, it works as expected, so I'm grateful.

Some comments:

1. I think that a small example in the readme would be helpful (usually, for understanding usage I'm going to the test cases, but from what I've seen, there is no easy example there).

   Maybe just something like:

   const status: LRU.Status<string> = {};
   cache.fetch(key, { status });
   console.log(status.fetch);

2. When using allowStaleOnFetchRejection and the fetchMethod fails, the resulting status is the same (shows fetch='rejected') for two different cases: when a stale value is returned from cache and when there is no existing value in the cache and there is an exception.

   This is not critical, as the flow can be deduced from the return value. But it would be nice if the status would help differentiate all possible flows.

3. When using allowStale and a stale value is returned from fetch(), the status only contains { fetch: 'stale' }.

   I'm not sure if this is the intention, but it would be nice if it could show some properties (e.g. a negative ttl value) of the stale value.

   Again, this is not critical.

4. Design issue: Currently you expose the states (e.g. on fetch field) according to the implementation. This might prove problematic in the future if the implementation will change, as clients might start relying on the specific states. Moreover, it requires the clients to understand these states. For example, for knowing whether a stale value was returned from fetch(), I'll need to check for both rejected and stale states (assuming both allowStale and allowStaleOnFetchRejection are set).

   An easy solution, at least for the first problem, is having a note on the readme that clients should not rely on the states as they might change in the future.

   A more robust solution (from my PoV), is having numerous boolean fields that describes the state. For example, instead of a single fetch field, there could be the fields fetchInflight, fetchResolved, etc. This way, a client that is just interested in knowing whether a fetch was resolved, will only look at the field fetchResolved which would be true for both get, resolved and refresh states.

5. I wrote a comment on one of the commits (there is a merge leftover line).

   How come the unit tests didn't detect it?

[isaacs]

Thanks for the feedback.

I think you're right, booleans might be better for some of these than strings, or at least, for fetch. For get/has, just having hit | miss | stale is fine, and then if it's set to stale, I can tack on the TTL info, and allowStale:boolean if it was returned or not. So if you see {get:'stale', allowStale:true} you know it got returned.

For fetch(), maybe it should be the same {fetch: hit | miss | stale}, to reflect the initial check, and then add other booleans for whether it entered a given state and what the result was. So in the case of ignoreFetchAbort+allowStaleOnFetchAbort, you might end up with something like:

{
  "fetch": "stale",
  "fetchRequested": true,
  "fetchAborted": true,
  "allowStale": true
}

to indicate that the item in cache was stale, so we made a request, which was aborted, and so we returned the stale value (allowStale being true). If you keep looking at the object, eventually it'll get updated to:

{
  "fetch": "stale",
  "fetchRequested": true,
  "fetchAborted": true,
  "allowStale": true,
  "fetchResolved": true,
  "fetchUpdated": true
}

to indicate that, even though the fetch was aborted and a stale value returned, it did eventually resolve and update the cache.

What do you think?

[isaacs]

The main thing I think you highlighted is that a given field shouldn't change its value, otherwise that's just inviting abuse of the API. They should be treated as immutable.

[yoshigev]

Great! The combination of string state plus additional boolean indications is awesome.

One small remark - I think using allowStale as one of the fields in the status might be confusing with the option that was set by the client. I think usedStale or returnedStale is clearer.

Thanks again!

[isaacs]

Ah, yeah, returnedStale is clearer, I agree.

[yoshigev]

The main thing I think you highlighted is that a given field shouldn't change its value, otherwise that's just inviting abuse of the API. They should be treated as immutable.

It was not what I meant, but it's a good point. Although strictly speaking, setting new values to the boolean fields also changes the status object underneath the legs of client. But as it is documented, it seems ok to me.

[isaacs]

setting new values to the boolean fields also changes the status object underneath the legs of client.

Haha, true, it does. But I guess if we're passing an object by-reference to an async API and then updating it at some point in the future, we're well into that territory already 😅 I guess "append only" is more what I was thinking, rather than "immutable".

[isaacs]

@yoshigev Updated with the changes discussed above. PTAL, if you have time. I'll sit on this for a bit and likely publish it in the next few days, assuming no other issues come up. You can try it out in your app by running:

npm install github:isaacs/node-lru-cache#pull/275

[yoshigev]

Hi @isaacs, We tried the new version and it works perfectly for our needs. Many thanks!