fix(packument): eliminate _cached field

[jablko]

I'm interested in telling whether the result of pacote.manifest() came from the registry or from the cacache. Is that what the existing packument _cached field indicates?

pacote/lib/registry.js

Line 77 in 298df45

packument._cached = res.headers.has('x-local-cache')

If so, does it make sense to add that field to pacote.manifest() results, and is this PR the correct way/location to do it?

[wraithgar]

This seems reasonable. Can we add an assertion to one of the registry.js tests that ensures this got set?

[jablko]

@wraithgar Thanks for the feedback! However when I went to write the test I realized this won't work, for two reasons:

1. I'd also need a _cached field on ETARGET errors, to tell whether those negative responses came from the cache or the network. pacote doesn't cache negative packuments (E404, at least I don't think it does) but it does cache negative manifests (package versions). So in my code I think I'm better off using the offline option and uniformly suppressing ENOTCACHED and ETARGET errors, i.e.:

   const _cached = await pacote
     .manifest("[email protected]", { cache, offline: true })
     .catch((reason) => {
       if (reason.code !== "ENOTCACHED" && reason.code !== "ETARGET") throw reason;
     });

2. The _cached field doesn't do what I thought it did. It might not do anything at all? Since maybe feat(BREAKING): refactor cache handling to be more consistent make-fetch-happen#49, maybe before that, I haven't confirmed (it doesn't look like addCacheHeaders() was called on misses before feat(BREAKING): refactor cache handling to be more consistent make-fetch-happen#49?) make-fetch-happen adds the X-Local-Cache header to cache misses as well as hits, so the _cached field is always true?

   Rather than fixing/restoring the _cached field, I now think I should either leave it alone (in which case I'll close this PR) or remove it completely (I've updated this PR to do that).

[wraithgar]

_cached doesn't mean that the request was fed by the cache, but that it was stored in the cache. I agree this doesn't appear to be very useful information, and there doesn't seem to be anywhere in the npm cli itself that we are evaluating this attribute. Unfortunately removing it is a semver major change. Perhaps the readme could be updated to clarify the distinction and we put a pin in either fixing it or removing it.

[wraithgar]

Made an issue to track #183

Keep this PR open, we may just decide that removing it is a bugfix since it's a _ attribute and was up until the last version undocumented.

[jablko]

@wraithgar Awesome thanks! +1 for removing it is a bugfix.

[lukekarrys]

@wraithgar This would be a good time to evaluate this change as we are on the verge of a major release of pacote?

[wraithgar]

@lukekarrys I checked again and we do not use this attribute.