Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly add stream to cache on cache add #2976

Closed
wants to merge 1 commit into from

Conversation

mjsir911
Copy link
Contributor

Pacote doesn't do this automatically anymore

Fixes npm/pacote#73 & Fixes #2160

@mjsir911 mjsir911 requested a review from a team as a code owner March 27, 2021 22:48
@mjsir911 mjsir911 force-pushed the msirabella/fix_cache branch 4 times, most recently from d93af78 to 41a60c8 Compare March 28, 2021 00:30
@mjsir911 mjsir911 changed the title Explicitly add stream to cache Explicitly add stream to cache on cache add Mar 28, 2021
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes Needs Review labels Mar 29, 2021
@darcyclarke darcyclarke added the release: next These items should be addressed in the next release label Mar 31, 2021
@isaacs
Copy link
Contributor

isaacs commented Mar 31, 2021

This is a little bit complicated.

Pacote will tee things into the cache if it's a remote fetch. If it's local, you're right, since the rewrite it just says "oh, you want a tarball? This is a tarball. Here you go!" and saves the extra work of copying the file from one place on disk to another place on disk.

So, two changes and one question. The changes:

  1. It should only explicitly pipe into cacache if it's a file: or git specifier. If it's a registry, remote, or alias spec, don't bother. (This can be determined by using npm-package-arg to parse the specifier. const npa = require('npm-package-arg'); spec = npa(spec); if (spec.type === 'remote' || spec.registry) { just return the stream } else { pipe to cacache }
  2. Cacache's folder is not just npm.config.get('cache'). We tack a _cacache path portion onto it. So you'll have to do something like the const cachePath = path.join(this.npm.cache, '_cacache') that appears elsewhere in the file, and pass that to cacache instead.

The question I have is, why do you need to add a file to the cache which is already present on disk? I'm curious about your use case.

The answer may well be that we should have an option to pacote to say "no, really, pipe this into the cache even if you don't think you have to", because the CLI should ideally remain somewhat blissfully unaware of exactly how various types of packages get handled. That is pacote's job.

@mjsir911
Copy link
Contributor Author

mjsir911 commented Mar 31, 2021 via email

@wraithgar wraithgar removed the release: next These items should be addressed in the next release label Mar 31, 2021
Pacote doesn't do this automatically anymore

Closes npm/pacote#73 & Closes npm#2160
@mjsir911
Copy link
Contributor Author

mjsir911 commented Apr 1, 2021

With the new changes I agree that this logic probably shouldn't be in npm-cli, feel free to close & I'll take a look at hacking onto pacote

@darcyclarke darcyclarke closed this Apr 1, 2021
mjsir911 added a commit to mjsir911/pacote that referenced this pull request Apr 1, 2021
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 npm#73 & Fixes npm/cli#2160

Also see npm/cli#2976 for my first attempt & some discussions as to how
we got here.
isaacs pushed a commit to npm/pacote that referenced this pull request Apr 13, 2021
Tell `fetch` not to cache it's downloaded tarballs, leave that to us

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

Fix: #73
Fix: npm/cli#2160
Close: npm/cli#2976

PR-URL: #74
Credit: @mjsir911
Close: #74
Reviewed-by: @isaacs

EDIT(@isaacs): Updated to add test, allow make-fetch-happen to use the
cache, pipe into cache properly, and avoid cache double-writes.
isaacs pushed a commit to npm/pacote that referenced this pull request Apr 13, 2021
Tell `fetch` not to cache it's downloaded tarballs, leave that to us

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

Fix: #73
Fix: npm/cli#2160
Close: npm/cli#2976

PR-URL: #74
Credit: @mjsir911
Close: #74
Reviewed-by: @isaacs

EDIT(@isaacs): Updated to add test, allow make-fetch-happen to use the
cache, pipe into cache properly, and avoid cache double-writes.
isaacs pushed a commit to npm/pacote that referenced this pull request Apr 15, 2021
Tell `fetch` not to cache it's downloaded tarballs, leave that to us

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

Fix: #73
Fix: npm/cli#2160
Close: npm/cli#2976

PR-URL: #74
Credit: @mjsir911
Close: #74
Reviewed-by: @isaacs

EDIT(@isaacs): Updated to add test, allow make-fetch-happen to use the
cache, pipe into cache properly, and avoid cache double-writes.
isaacs pushed a commit to npm/pacote that referenced this pull request Apr 15, 2021
Tell `fetch` not to cache it's downloaded tarballs, leave that to us

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

Fix: #73
Fix: npm/cli#2160
Close: npm/cli#2976

PR-URL: #74
Credit: @mjsir911
Close: #74
Reviewed-by: @isaacs

EDIT(@isaacs): Updated to add test, allow make-fetch-happen to use the
cache, pipe into cache properly, and avoid cache double-writes.
mjsir911 added a commit to mjsir911/cli that referenced this pull request Apr 18, 2021
This is a backwards incompatible change to the undocumented
`cache add pkg version`, but

Motivations for this can be found here:
npm#2976 (comment)

Signed-off-by: Marco Sirabella <marco@sirabella.org>
mjsir911 added a commit to mjsir911/cli that referenced this pull request Apr 18, 2021
This is a backwards incompatible change to the undocumented
`cache add pkg version`, but

Motivations for this can be found here:
npm#2976 (comment)

Signed-off-by: Marco Sirabella <marco@sirabella.org>
mjsir911 added a commit to mjsir911/cli that referenced this pull request Apr 18, 2021
This is a backwards incompatible change to the undocumented
`cache add pkg version`, but

Motivations for this can be found here:
npm#2976 (comment)

Signed-off-by: Marco Sirabella <marco@sirabella.org>
isaacs pushed a commit to npm/pacote that referenced this pull request Apr 22, 2021
Tell `fetch` not to cache it's downloaded tarballs, leave that to us

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

Fix: #73
Fix: npm/cli#2160
Close: npm/cli#2976
Close: #74

EDIT(@isaacs): Updated to add test, allow make-fetch-happen to use the
cache, pipe into cache properly, and avoid cache double-writes.

PR-URL: #75
Credit: @mjsir911, @isaacs
Close: #75
Reviewed-by: @wraithgar, @mjsir911
mjsir911 added a commit to mjsir911/cli that referenced this pull request May 4, 2021
This is a backwards incompatible change to the undocumented
`cache add pkg version`, but

Motivations for this can be found here:
npm#2976 (comment)

Signed-off-by: Marco Sirabella <marco@sirabella.org>
wraithgar pushed a commit to mjsir911/cli that referenced this pull request May 6, 2021
This is a backwards incompatible change to the undocumented
`cache add pkg version`, but

Motivations for this can be found here:
npm#2976 (comment)

Signed-off-by: Marco Sirabella <marco@sirabella.org>

PR-URL: npm#3098
Credit: @mjsir911
Close: npm#3098
Reviewed-by: @wraithgar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] pacote does not add to cache [BUG] cache add tarball file does not add to cache
4 participants