This repository has been archived by the owner. It is now read-only.

Replace metadata + fetch + cache code #15666

Closed
wants to merge 71 commits into
from

Conversation

Projects
None yet
7 participants
@zkat
Member

zkat commented Feb 3, 2017

Summary

This PR replaces everything in the current codebase having to do with "give me the metadata for this package" and "extract this package to this place" with a single library, pacote which abstracts away the whole thing. This includes all the caching code, since pacote handles all of that internally through cacache

This means the following bits are being entirely replaced:

  • fetchPackageMetadata
  • lib/cache
  • mapToRegistry
  • All calls to cache.* in various parts of the codebase
  • All bits in lib/doctor having to do with cache verification (cacache has its own verifier this can call)
  • others?

On the user side, the only breaking change will be that the cache structure will have completely changed, and some incidentals of cache management may end up getting tweaked, but by the time this is done, it should be mostly compatible.

Status

Initial registry dep support in pacote is sketched out, and integration can begin. Expect lots of loose ends and a lot of back-and-forth between the libraries as the API is sussed out in more detail.

UPDATE (2017-04-17):

This branch is now usable. It needs some fixes, and the test suite is still failing due to compatibility issues and probably-minor issues. It is not production ready, but it runs and it works. All the work on it is now going into getting the npm test suite passing, at which point we'll be able to do a final review + merge.

UPDATE (2017-03-14):

Currently, cacache is production-ready, fast, and really damn reliable.

Most work is currently focused on pacote, and finishing up and testing that all of its package types actually work. This should be done soon, and then this branch can start moving again. The only major component left is git support. pacote@1 will be tagged once that's done, and further versions added during integration and test fixing for the npm CLI test suite (which I haven't run and am kinda terrified about)

I want to help!

Cool! Head on over to the pacote and cacache git repositories and look in the issues for help-wanted and starter bits and comment there if you want to help with those.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 13, 2017

Coverage Status

Coverage decreased (-1.3%) to 84.744% when pulling f958602 on zkat/pacote into 892f531 on release-next.

Coverage Status

Coverage decreased (-1.3%) to 84.744% when pulling f958602 on zkat/pacote into 892f531 on release-next.

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Feb 13, 2017

Member

@zkat If we can get this patch as is, passing tests, I think I'd like to merge it before we move on to further integration. What do you think?

Member

iarna commented Feb 13, 2017

@zkat If we can get this patch as is, passing tests, I think I'd like to merge it before we move on to further integration. What do you think?

@zkat zkat removed the breaking label Feb 14, 2017

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 14, 2017

Member

@iarna https://github.com/zkat/pacote/milestone/2 There is now a dedicated milestone for pacote@0.1.0 defined as "whatever we need in order to replace the current registry-install stack with pacote".

Member

zkat commented Feb 14, 2017

@iarna https://github.com/zkat/pacote/milestone/2 There is now a dedicated milestone for pacote@0.1.0 defined as "whatever we need in order to replace the current registry-install stack with pacote".

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 17, 2017

Coverage Status

Coverage decreased (-22.0%) to 64.054% when pulling 6d09fef on zkat/pacote into 892f531 on release-next.

Coverage Status

Coverage decreased (-22.0%) to 64.054% when pulling 6d09fef on zkat/pacote into 892f531 on release-next.

@zkat zkat added the breaking label Apr 4, 2017

@zkat zkat referenced this pull request Apr 4, 2017

Closed

release: npm@5.0.0 #16244

16 of 24 tasks complete
Show outdated Hide outdated doc/misc/npm-config.md
### offline
* Default: false
* Type: Boolea

This comment has been minimized.

This comment has been minimized.

@zkat

zkat Apr 5, 2017

Member

lol nice catch. Thank you.

@zkat

zkat Apr 5, 2017

Member

lol nice catch. Thank you.

iarna and others added some commits Mar 1, 2017

dedupe: find-dupes should output _something_
npm find-dupes is now an alias for `npm dedupe --dry-run --parseable`
feat(opts): save installs by default
BREAKING CHANGE: you need to use --no-save to skip saves now;

zkat added some commits May 4, 2017

@simonua

This comment has been minimized.

Show comment
Hide comment
@simonua

simonua May 26, 2017

Contributor

@zkat I'm considering recommending npm cache clean to our teams prior to upgrading to npm5 to clean out the old pre-5 caches. Is there a better, more suggested way? An npm upgrade to 5 would not automatically wipe out those artifacts, would it?

Contributor

simonua commented May 26, 2017

@zkat I'm considering recommending npm cache clean to our teams prior to upgrading to npm5 to clean out the old pre-5 caches. Is there a better, more suggested way? An npm upgrade to 5 would not automatically wipe out those artifacts, would it?

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat May 26, 2017

Member

@simonua I would even recommend you keep the npm4 cache around until you've confirmed npm5 is currently stable enough on your own project. But cache clear is the way to go once you're ready -- you can also manually preserve ~/.npm/_cacache and delete everything else to only keep the npm5 cache

Member

zkat commented May 26, 2017

@simonua I would even recommend you keep the npm4 cache around until you've confirmed npm5 is currently stable enough on your own project. But cache clear is the way to go once you're ready -- you can also manually preserve ~/.npm/_cacache and delete everything else to only keep the npm5 cache

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan May 26, 2017

The npm 5 announcement says--cache-min and --cache-max have been deprecated (#15666)” underneath its Breaking Changes section.

But only one of these can be true:

  1. --cache-min and --cache-max have been removed, and this in fact constitutes a breaking change
  2. --cache-min and --cache-max have been deprecated, meaning that they still work but their use is discouraged with the intention that they will be removed, in which case this is not yet a breaking change

Which is it?

alanhogan commented May 26, 2017

The npm 5 announcement says--cache-min and --cache-max have been deprecated (#15666)” underneath its Breaking Changes section.

But only one of these can be true:

  1. --cache-min and --cache-max have been removed, and this in fact constitutes a breaking change
  2. --cache-min and --cache-max have been deprecated, meaning that they still work but their use is discouraged with the intention that they will be removed, in which case this is not yet a breaking change

Which is it?

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat May 26, 2017

Member

@alanhogan they have been deprecated, and their semantics have been limited due to the design of the underlying cache (which works entirely through http cache rules). The reason they haven't been removed is so that anyone who had things like --cache-min=999999 still work (it's essentially an alias for --prefer-offline. Likewise, --cache-max=0 is --prefer-online.

Furthermore, we consider the act of deprecation to be a breaking change, as a matter of policy. Deprecations might just not happen to break CI (they might, sometimes, if there's new warning output, though).

Member

zkat commented May 26, 2017

@alanhogan they have been deprecated, and their semantics have been limited due to the design of the underlying cache (which works entirely through http cache rules). The reason they haven't been removed is so that anyone who had things like --cache-min=999999 still work (it's essentially an alias for --prefer-offline. Likewise, --cache-max=0 is --prefer-online.

Furthermore, we consider the act of deprecation to be a breaking change, as a matter of policy. Deprecations might just not happen to break CI (they might, sometimes, if there's new warning output, though).

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat May 27, 2017

Member

This landed in npm@5.0.0 🎉

Member

zkat commented May 27, 2017

This landed in npm@5.0.0 🎉

@zkat zkat closed this May 27, 2017

@zkat zkat removed the in-progress label May 27, 2017

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan May 27, 2017

Excellent explanation, thank you.

Excellent explanation, thank you.

@rally25rs

This comment has been minimized.

Show comment
Hide comment
@rally25rs

rally25rs May 31, 2017

Was cache ls intentionally removed during this cache rewrite? Is there a replacement command?

$ npm --version
5.0.0

$ npm cache ls
npm ERR! Usage: npm cache add <tarball file>
npm ERR! npm cache add <folder>
npm ERR! npm cache add <tarball url>
npm ERR! npm cache add <git url>
npm ERR! npm cache add <name>@<version>
npm ERR! npm cache clean
npm ERR! npm cache verify

If so, docs should probably be updated now that v5 is out
https://docs.npmjs.com/cli/cache

Was cache ls intentionally removed during this cache rewrite? Is there a replacement command?

$ npm --version
5.0.0

$ npm cache ls
npm ERR! Usage: npm cache add <tarball file>
npm ERR! npm cache add <folder>
npm ERR! npm cache add <tarball url>
npm ERR! npm cache add <git url>
npm ERR! npm cache add <name>@<version>
npm ERR! npm cache clean
npm ERR! npm cache verify

If so, docs should probably be updated now that v5 is out
https://docs.npmjs.com/cli/cache

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat May 31, 2017

Member

@rally25rs https://github.com/npm/npm/blob/latest/doc/cli/npm-cache.md the docs are updated, but there's issues with the auto-deploy scripts that push them to the site. The removal was intentional: the cache structure changed so much that it's gonna take a bit before there can be a replacement.

Member

zkat commented May 31, 2017

@rally25rs https://github.com/npm/npm/blob/latest/doc/cli/npm-cache.md the docs are updated, but there's issues with the auto-deploy scripts that push them to the site. The removal was intentional: the cache structure changed so much that it's gonna take a bit before there can be a replacement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.