Cache tarballs streamed through pacote

[mjsir911]

Tell fetch not to cache it's downloaded tarballs, leave that to us

Not sure the implications of changing the return type of _istream / adding a cacache to the end of the pipe, too unfamiliar with streams

Fixes #73 & Fixes npm/cli#2160

Also see npm/cli#2976 for my first attempt & some discussions as to how we got here.

Also this behavior should probably be documented somewhere so it doesn't slip away again 😅

[mjsir911]

Assuming the pipe-ing is somewhat correct, would a MinipassPipeline be better here?

[isaacs]

Yeah, I think this should almost certainly be return new Pipeline(stream, istream, cstream)

[isaacs]

Er, wait, no. I think cstream might not be a Passthrough?

I'll pull this locally and poke at it a bit, sorry for the uninformed comment 😅

[isaacs]

Ok, I have a fix for this, will squash into the commit when landing.

Suggested change

	return stream.on('error', er => istream.emit('error', er)).pipe(istream).pipe(cstream)
	const pipeline = new Pipeline(stream, istream)
	stream.pipe(cacache.put.stream(
	this.opts.cache,
	`pacote:tarball:${this.from}`,
	this.opts
	))
	return pipeline

[isaacs]

So, in principle, I think this is probably the right direction. We still let mfh do the caching of everything other than tarballs, but explicitly cache tarballs in the single choke-point in the logical flow.

I'll pull locally and take a look now. At the very least, it will need tests to assert that file: and other tarballs are being cached as expected.

Thanks for updating your use case information over in npm/cli#2976 (comment) It's helpful to know the end goal, and that does make sense. We may want to update npm cache add <spec> to take multiple specs and add them all, I don't think that would be too terrible of a CLI change, and would likely make your bash script quite a bit faster.

[isaacs]

Marking as "request changes", but I have the required changes ready to land, so don't sweat it. I'll open up a separate PR to review, and mark this as closed once that lands.

Thanks!

[isaacs]

Ok, I have a fix for this, will squash into the commit when landing.

Suggested change

	return stream.on('error', er => istream.emit('error', er)).pipe(istream).pipe(cstream)
	const pipeline = new Pipeline(stream, istream)
	stream.pipe(cacache.put.stream(
	this.opts.cache,
	`pacote:tarball:${this.from}`,
	this.opts
	))
	return pipeline

[isaacs]

This will also prevent make-fetch-happen from seeing the tarballs that pacote has cached. The options seem to be:

1. Accept extra requests being made when the tarball already exists in cache (not really acceptable)
2. Let it write twice into the cache (cacache prevents data integrity problems by using atomic writes, but still not great, because it's double disk work when fetching a remote tarball)
3. Only pipe into the cstream for fetchers that don't expect their fetches to be cached anyway (a little bit dirty, because it requires that the fetcher knows what mfh is going to do)
4. Provide a cacheManager object that reads from cache, but only writes packuments to cache, not tarballs (probably the "cleanest" solution, but... oof, that's a lot of duplicated code).

I'm back to thinking (3) makes the most sense here.

[isaacs]

@mjsir911 Can you take a look at #75?

[isaacs]

Closed in favor of #75