Fix #995 Optional Dependencies

4 files changed, 67 insertions, 34 deletions

diff --git a/lib/install.js b/lib/install.js
index ca54585e0..65bd5bb5d 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -118,11 +118,9 @@ function install (args, cb_) {
         installManyTop(deps.map(function (dep) {
           var target = data.dependencies[dep]
             , parsed = url.parse(target.replace(/^git\+/, "git"))
-          if (!parsed.protocol) {
-            target = dep + "@" + target
-          }
+          target = dep + "@" + target
           return target
-        }), where, family, ancestors, false, cb)
+        }), where, family, ancestors, false, data, cb)
       })
     }
 
@@ -134,7 +132,7 @@ function install (args, cb_) {
         , ancestors = {}
       if (data) family[data.name] = ancestors[data.name] = data.version
       var fn = npm.config.get("global") ? installMany : installManyTop
-      fn(args, where, family, ancestors, true, cb)
+      fn(args, where, family, ancestors, true, data, cb)
     })
   })
 }
@@ -151,7 +149,6 @@ function save (where, installed, tree, pretty, cb) {
   // The relevant tree shape is { <folder>: {what:<pkg>} }
   var saveTarget = path.resolve(where, "package.json")
     , things = Object.keys(tree).map(function (k) {
-        //log.warn(k, "k")
         return tree[k].what.split("@")
       }).reduce(function (set, k) {
         var rangeDescriptor = semver.gte(k[1], "0.1.0") ? "~" : ""
@@ -159,8 +156,6 @@ function save (where, installed, tree, pretty, cb) {
         return set
       }, {})
 
-  //log.warn(things, "things")
-
   // don't use readJson, because we don't want to do all the other
   // tricky npm-specific stuff that's in there.
   fs.readFile(saveTarget, function (er, data) {
@@ -257,7 +252,7 @@ function treeify (installed) {
 
 // just like installMany, but also add the existing packages in
 // where/node_modules to the family object.
-function installManyTop (what, where, family, ancestors, explicit, cb_) {
+function installManyTop (what, where, family, ancestors, explicit, parent, cb_) {
 
   function cb (er, d) {
     if (explicit || er) return cb_(er, d)
@@ -277,18 +272,18 @@ function installManyTop (what, where, family, ancestors, explicit, cb_) {
 
   function next (er) {
     if (er) return cb(er)
-    installManyTop_(what, where, family, ancestors, explicit, cb)
+    installManyTop_(what, where, family, ancestors, explicit, parent, cb)
   }
 }
 
-function installManyTop_ (what, where, family, ancestors, explicit, cb) {
+function installManyTop_ (what, where, family, ancestors, explicit, parent, cb) {
   var nm = path.resolve(where, "node_modules")
     , names = explicit
             ? what.map(function (w) { return w.split(/@/).shift() })
             : []
 
   fs.readdir(nm, function (er, pkgs) {
-    if (er) return installMany(what, where, family, ancestors, explicit, cb)
+    if (er) return installMany(what, where, family, ancestors, explicit, parent, cb)
     pkgs = pkgs.filter(function (p) {
       return !p.match(/^[\._-]/)
           && (!explicit || names.indexOf(p) === -1)
@@ -306,12 +301,12 @@ function installManyTop_ (what, where, family, ancestors, explicit, cb) {
       packages.forEach(function (p) {
         family[p[0]] = p[1]
       })
-      return installMany(what, where, family, ancestors, explicit, cb)
+      return installMany(what, where, family, ancestors, explicit, parent, cb)
     })
   })
 }
 
-function installMany (what, where, family, ancestors, explicit, cb) {
+function installMany (what, where, family, ancestors, explicit, parent, cb) {
   // 'npm install foo' should install the version of foo
   // that satisfies the dep in the current folder.
   // This will typically return immediately, since we already read
@@ -320,15 +315,19 @@ function installMany (what, where, family, ancestors, explicit, cb) {
     if (er) data = {}
 
     d = data.dependencies || {}
-    var parent = data._id
+    var parent = data
 
-    log.verbose(what, "into "+where)
     // what is a list of things.
     // resolve each one.
     asyncMap( what
-            , targetResolver(where, family, ancestors, explicit, d)
+            , targetResolver(where, family, ancestors, explicit, d, parent)
             , function (er, targets) {
-      if (er) return cb(er)
+
+      if (er) {
+        log.error(er, "target resolver error")
+        return cb(er)
+      }
+
       // each target will be a data object corresponding
       // to a package, folder, or whatever that is in the cache now.
       var newPrev = Object.create(family)
@@ -350,7 +349,7 @@ function installMany (what, where, family, ancestors, explicit, cb) {
   })
 }
 
-function targetResolver (where, family, ancestors, explicit, deps) {
+function targetResolver (where, family, ancestors, explicit, deps, parent) {
   var alreadyInstalledManually = explicit ? [] : null
     , nm = path.resolve(where, "node_modules")
 
@@ -389,8 +388,13 @@ function targetResolver (where, family, ancestors, explicit, deps) {
     if (deps[what]) {
       what = what + "@" + deps[what]
     }
-    log.verbose(what, "cache add")
+
     cache.add(what, function (er, data) {
+      if (er && parent && parent.optionalDependencies &&
+          parent.optionalDependencies.hasOwnProperty(what.split("@").shift())) {
+        log.warn(what, "optional dependency failed, continuing")
+        return cb(null, [])
+      }
       if (!er && data && family[data.name] === data.version) {
         return cb(null, [])
       }
@@ -430,7 +434,7 @@ function localLink (target, where, family, ancestors, parent, cb) {
       function thenLink () {
         npm.commands.link([target.name], function (er, d) {
           log.silly([er, d], "back from link")
-          cb(er, [resultList(target, where, parent)])
+          cb(er, [resultList(target, where, parent && parent._id)])
         })
       }
 
@@ -441,7 +445,7 @@ function localLink (target, where, family, ancestors, parent, cb) {
   })
 }
 
-function resultList (target, where, parent) {
+function resultList (target, where, parentId) {
   var nm = path.resolve(where, "node_modules")
     , targetFolder = path.resolve(nm, target.name)
     , prettyWhere = relativize(where, process.cwd() + "/x")
@@ -456,8 +460,8 @@ function resultList (target, where, parent) {
 
   return [ target._id
          , targetFolder
-         , prettyWhere && parent
-         , parent && prettyWhere ]
+         , prettyWhere && parentId
+         , parentId && prettyWhere ]
 }
 
 function installOne_ (target, where, family, ancestors, parent, cb) {
@@ -475,8 +479,7 @@ function installOne_ (target, where, family, ancestors, parent, cb) {
     , function (er, d) {
         log.verbose(target._id, "installOne cb")
         if (er) return cb(er)
-
-        d.push(resultList(target, where, parent))
+        d.push(resultList(target, where, parent && parent._id))
         cb(er, d)
       }
     )
@@ -591,12 +594,9 @@ function write (target, targetFolder, family, ancestors, cb_) {
       }).map(function (d) {
         var t = target.dependencies[d]
           , parsed = url.parse(t.replace(/^git\+/, "git"))
-        if (!parsed.protocol) {
-          t = d + "@" + t
-        }
+        t = d + "@" + t
         return t
-      }), targetFolder, family, ancestors, false, function (er, d) {
-        //log.warn(d, "write installMany cb")
+      }), targetFolder, family, ancestors, false, target, function (er, d) {
         log.verbose(targetFolder, "about to build")
         if (er) return cb(er)
         npm.commands.build( [targetFolder]
diff --git a/lib/utils/read-installed.js b/lib/utils/read-installed.js
index 59fc6c490..6c0ece25b 100644
--- a/lib/utils/read-installed.js
+++ b/lib/utils/read-installed.js
@@ -196,6 +196,16 @@ function readInstalled_ (folder, parent, name, reqver, depth, maxDepth, cb) {
       installedData.forEach(function (dep) {
         obj.dependencies[dep.realName] = dep
       })
+
+      // any strings here are unmet things.  however, if it's
+      // optional, then that's fine, so just delete it.
+      if (obj.optionalDependencies) {
+        Object.keys(obj.optionalDependencies).forEach(function (dep) {
+          if (typeof obj.dependencies[dep] === "string") {
+            delete obj.dependencies[dep]
+          }
+        })
+      }
       return cb(null, obj)
     })
   }
@@ -253,6 +263,7 @@ function findUnmet (obj) {
         }
         deps[d] = found
       }
+
     })
   log.verbose([obj._id], "returning")
   return obj
diff --git a/lib/utils/read-json.js b/lib/utils/read-json.js
index ffd447d32..388d6727e 100644
--- a/lib/utils/read-json.js
+++ b/lib/utils/read-json.js
@@ -362,11 +362,23 @@ function processObject (opts, cb) { return function (er, json) {
     delete json["dev-dependencies"]
   }
 
-  ;["dependencies", "devDependencies"].forEach(function (d) {
-    json[d] = json.hasOwnProperty(d) ? depObjectify(json[d], d, json._id) : {}
+  ; [ "dependencies"
+    , "devDependencies"
+    , "optionalDependencies"
+    ].forEach(function (d) {
+      json[d] = json.hasOwnProperty(d)
+              ? depObjectify(json[d], d, json._id)
+              : {}
+    })
+
+  // always merge optionals into deps
+  Object.keys(json.optionalDependencies).forEach(function (d) {
+    json.dependencies[d] = json.optionalDependencies[d]
   })
 
-  if (opts.dev || npm.config.get("dev") || npm.config.get("npat")) {
+  if (opts.dev
+      || npm.config.get("dev")
+      || npm.config.get("npat")) {
     // log.warn(json._id, "Adding devdeps")
     Object.keys(json.devDependencies || {}).forEach(function (d) {
       json.dependencies[d] = json.devDependencies[d]
diff --git a/test/packages/npm-test-optional-deps/package.json b/test/packages/npm-test-optional-deps/package.json
new file mode 100644
index 000000000..ebcd56832
--- /dev/null
+++ b/test/packages/npm-test-optional-deps/package.json
@@ -0,0 +1,10 @@
+{ "name": "npm-test-optional-deps"
+, "version": "1.2.5"
+, "optionalDependencies":
+  { "npm-test-foobarzaaakakaka": "http://example.com/"
+  , "dnode": "10.999.14234"
+  , "sax": "*"
+  , "999 invalid name": "1.2.3"
+  , "glob": "some invalid version 99 #! $$ x y z"
+  }
+}