Permalink
Browse files

fix #3046 --save properly when installing github shorthands

  • Loading branch information...
1 parent 89216f3 commit 8f77ea9cbe2ce76df070d637ec9e5eb35a16b3bd @isaacs isaacs committed Jan 17, 2013
Showing with 8 additions and 10 deletions.
  1. +7 −10 lib/cache.js
  2. +1 −0 lib/install.js
View
@@ -545,7 +545,7 @@ function addNamed (name, x, data, cb_) {
if (iF.length > 1) return
function cb (er, data) {
- if (data) data._from = k
+ if (data && !data._fromGithub) data._from = k
unlock(k, function () {
var c
while (c = iF.shift()) c(er, data)
@@ -753,7 +753,7 @@ function addLocal (p, name, cb_) {
log.error("addLocal", "Could not install %s", p)
return cb_(er)
}
- data._from = p
+ if (data && !data._fromGithub) data._from = p
return cb_(er, data)
})
}
@@ -777,17 +777,14 @@ function addLocal (p, name, cb_) {
}
function maybeGithub (p, name, er, cb) {
- var u = "https://github.com/" + p
+ var u = "git://github.com/" + p
@ctalkington

ctalkington Jan 23, 2013

is there a specific reason why the ability to install from a GH tarball was removed?

@edef1c

edef1c Jan 23, 2013

Member
<isaacs> oh, that's because i switched it back, sorry.  we can put it back to doing the .tgz thing, but it wasn't saving properly, and the easiest way to make it save properly was just to make it transform to git:// 
<isaacs> it also caches git remotes now, so if you use the same github repo a lot, it'll be faster to actually use git rather than tarball
<isaacs> also, this means that shrinkwrap can turn "nathan7/npm#master" into git://github.com/nathan7/npm/#0cead032affead
<isaacs> in the _resolved field
@ctalkington

ctalkington Jan 23, 2013

hum ok. maybe it wasn't working cause url changed on GH if I recall:

user/repo/archive/{branch or commitish}.tar.gz

the new things are cool but maybe it should do both by checking for git in PATH vs assuming it exists.

@edef1c

edef1c Jan 23, 2013

Member

I'm somewhat wary of doing magic behaviour based on whether we can find shit in PATH (windows is rather wonky with this) but it seems like an option. There are plans to refactor all this code so we'll probably work it in when we do that.

@isaacs

isaacs Jan 23, 2013

Owner

Here's the issue:

Let's say that you install user/project#master. Then you do npm shrinkwrap. I'd like to know that you shrinkwrap the exact commit where user/project#master points at, so that even if the user pushes more stuff, the shrinkwrapped package won't change.

That's not possible unless we stash the resolved commit data, and we can't even get that if we download from a tarball. Also, git remotes are cached now, so the speed is not as much of an issue (and that was the original motivation to go with the download tarballs).

I'd be happy with a fallback in addRemoteGit that fetches from a tarball if the host is github.

@ctalkington

ctalkington Jan 24, 2013

got it, maybe it could just listen for a fail on the process exec of git. #3089 has example of error windows throws.

, up = url.parse(u)
- if (up.hash && up.hash[0] === "#")
- up.hash = up.hash.slice(1)
-
- var ref = encodeURIComponent(up.hash || "master")
- up.pathname = path.join(up.pathname, "tarball", ref).replace(/\\/g, "/")
- u = url.format(up)
log.info("maybeGithub", "Attempting to fetch %s from %s", p, u)
- return addRemoteTarball(u, null, name, function (er2, data) {
+
+ return addRemoteGit(u, up, name, function (er2, data) {
if (er2) return cb(er)
+ data._from = u
+ data._fromGithub = true
return cb(null, data)
})
}
View
@@ -305,6 +305,7 @@ function save (where, installed, tree, pretty, cb) {
data.bundleDependencies = bundle
}
+ log.verbose('saving', things)
data[deps] = data[deps] || {}
Object.keys(things).forEach(function (t) {
data[deps][t] = things[t]

0 comments on commit 8f77ea9

Please sign in to comment.