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

Already on GitHub? Sign in to your account

cache.js refactor: Introduce a git module #4824

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
Member

robertkowalski commented Mar 2, 2014

This PR enables the standalone testing of our git processes while installing from git (see also, but not only #4667 - Don't *always* fetch remote repositories of git-dependencies)

It also factors out almost the whole process of fetching tarballs from git into an own module.

I am happy to rebase/squash this, but as there were some refactoring I leave the single commits for an easier review.

Notes:

  • I did not include locking in the module
  • In the future we might want to return a stream with the tarball in npm-cache-git but it wasn't in the scope now
Member

robertkowalski commented Mar 7, 2014

@isaacs any opinion on that before the weekend starts?

Contributor

guybrush commented Mar 7, 2014

sweet! factoring out the git-cache into its own module is very nice :)

actually factoring out the whole cache would be even more nice (npm depending on npm-cache depending on npm-git-cache).

Member

domenic commented Apr 7, 2014

This tentatively LGTM, assuming it rebases nicely. @isaacs?

Owner

isaacs commented Apr 13, 2014

So, I'm a bit leery about refactoring the cache folder into multiple modules without taking some time to think about where we're going. It runs the risk of not actually making things better, but just splitting the logic and data arbitrarily into multiple places. (Qv. npmconf module for why that can be somewhat awful, if not done carefully.)

This module is still tightly coupled to npm's very specific logic, and by being tightly coupled in multiple modules, it's even more difficult to change.

Rather than copy out the functions exactly as they are in npm's internals, it would be better, I think, to try to figure out what sort of general-purpose utility would have made npm's internals simpler, and then go build THAT thing.

@isaacs isaacs closed this Apr 13, 2014

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