diff options
author | Forrest L Norvell <forrest@npmjs.com> | 2014-05-03 04:59:39 +0400 |
---|---|---|
committer | Forrest L Norvell <forrest@npmjs.com> | 2014-05-08 05:00:51 +0400 |
commit | 235002da9d22ff6ea24ec3e25a2e2a0e441caf55 (patch) | |
tree | 6483d3791e5bbe4cc7c3f4ea94b8ed6cf41073e3 | |
parent | ff35ed6b67489067a31f112d026dddfcf51b8929 (diff) |
refactor maybe* methods to be more consistent
-rw-r--r-- | lib/cache.js | 80 | ||||
-rw-r--r-- | lib/cache/add-local.js | 2 | ||||
-rw-r--r-- | lib/cache/add-named.js | 31 | ||||
-rw-r--r-- | lib/cache/maybe-github.js | 11 | ||||
-rw-r--r-- | test/tap/maybe-github.js | 76 |
5 files changed, 125 insertions, 75 deletions
diff --git a/lib/cache.js b/lib/cache.js index 1ff8718ba..8038e222f 100644 --- a/lib/cache.js +++ b/lib/cache.js @@ -127,6 +127,7 @@ function read (name, ver, forceBypass, cb) { var jsonFile = path.join(npm.cache, name, ver, "package", "package.json") function c (er, data) { if (data) deprCheck(data) + return cb(er, data) } @@ -140,7 +141,9 @@ function read (name, ver, forceBypass, cb) { er = needVersion(er, data) if (er && er.code !== "ENOENT" && er.code !== "ENOTDIR") return cb(er) if (er) return addNamed(name, ver, null, c) + deprCheck(data) + c(er, data) }) } @@ -251,15 +254,27 @@ function add (args, cb) { // in that case, we will not have a protocol now, but if we // split and check, we will. if (!name && !p.protocol) { - if (spec.indexOf("/") !== -1 || - process.platform === "win32" && spec.indexOf("\\") !== -1) { - return maybeFile(spec, p, cb) - } else if (spec.indexOf("@") !== -1) { - return maybeAt(spec, cb) + return maybeFile(spec, cb) + } + else { + switch (p.protocol) { + case "http:": + case "https:": + return addRemoteTarball(spec, null, name, "", cb) + + default: + if (isGitUrl(p)) return addRemoteGit(spec, p, false, cb) + + // if we have a name and a spec, then try name@spec + if (name) { + addNamed(name, spec, null, cb) + } + // if not, then try just spec (which may try name@"" if not found) + else { + addLocal(spec, "", cb) + } } } - - add_(name, spec, p, cb) } function unpack (pkg, ver, unpackTarget, dMode, fMode, uid, gid, cb) { @@ -284,26 +299,6 @@ function unpack (pkg, ver, unpackTarget, dMode, fMode, uid, gid, cb) { }) } -function add_ (name, spec, p, cb) { - switch (p.protocol) { - case "http:": - case "https:": - return addRemoteTarball(spec, null, name, "", cb) - - default: - if (isGitUrl(p)) - return addRemoteGit(spec, p, false, cb) - - // if we have a name and a spec, then try name@spec - // if not, then try just spec (which may try name@"" if not found) - if (name) { - addNamed(name, spec, null, cb) - } else { - addLocal(spec, "", cb) - } - } -} - function afterAdd (arg, cb) { return function (er, data) { if (er || !data || !data.name || !data.version) { return cb(er, data) @@ -319,29 +314,30 @@ function afterAdd (arg, cb) { return function (er, data) { }) }} -function maybeFile (spec, p, cb) { +function maybeFile (spec, cb) { + // split name@2.3.4 only if name is a valid package name, + // don't split in case of "./test@example.com/" (local path) fs.stat(spec, function (er) { if (!er) { // definitely a local thing - addLocal(spec, "", cb) - } else if (er && spec.indexOf("@") !== -1) { - // bar@baz/loofa - maybeAt(spec, cb) - } else { - // Already know it's not a url, so must be local - addLocal(spec, "", cb) + return addLocal(spec, "", cb) } + + maybeAt(spec, cb) }) } function maybeAt (spec, cb) { - var tmp = spec.split("@") - - // split name@2.3.4 only if name is a valid package name, - // don't split in case of "./test@example.com/" (local path) - var name = tmp.shift() - spec = tmp.join("@") - return add([name, spec], cb) + if (spec.indexOf("@") !== -1) { + var tmp = spec.split("@") + + var name = tmp.shift() + spec = tmp.join("@") + add([name, spec], cb) + } else { + // already know it's not a url, so must be local + addLocal(spec, "", cb) + } } function needName(er, data) { diff --git a/lib/cache/add-local.js b/lib/cache/add-local.js index 1bb6a6dbe..7b5923574 100644 --- a/lib/cache/add-local.js +++ b/lib/cache/add-local.js @@ -52,7 +52,7 @@ function addLocal (p, name, cb_) { // might be username/project // in that case, try it as a github url. if (p.split("/").length === 2) { - return maybeGithub(p, name, er, cb) + return maybeGithub(p, er, cb) } return cb(er) } diff --git a/lib/cache/add-named.js b/lib/cache/add-named.js index 2ef06eda1..caeb7cc24 100644 --- a/lib/cache/add-named.js +++ b/lib/cache/add-named.js @@ -15,8 +15,8 @@ var path = require("path") , locker = require("../utils/locker.js") , lock = locker.lock , unlock = locker.unlock + , maybeGithub = require("./maybe-github.js") , addRemoteTarball = require("./add-remote-tarball.js") - , addRemoteGit = require("./add-remote-git.js") module.exports = addNamed @@ -62,7 +62,7 @@ function addNameTag (name, tag, data, cb_) { // might be username/project // in that case, try it as a github url. if (er && tag.split("/").length === 2) { - return maybeGithub(tag, name, er, cb_) + return maybeGithub(tag, er, cb_) } return cb_(er, data) } @@ -239,33 +239,6 @@ function addNameRange (name, range, data, cb) { } } -function maybeGithub (p, name, er, cb) { - var u = "git://github.com/" + p - , up = url.parse(u) - log.info("maybeGithub", "Attempting %s from %s", p, u) - - return addRemoteGit(u, up, true, function (er2, data) { - if (er2) { - var upriv = "git+ssh://git@github.com:" + p - , uppriv = url.parse(upriv) - - log.info("maybeGithub", "Attempting %s from %s", p, upriv) - - return addRemoteGit(upriv, uppriv, false, function (er3, data) { - if (er3) return cb(er) - success(upriv, data) - }) - } - success(u, data) - }) - - function success (u, data) { - data._from = u - data._fromGithub = true - return cb(null, data) - } -} - function installTargetsError (requested, data) { var targets = Object.keys(data["dist-tags"]).filter(function (f) { return (data.versions || {}).hasOwnProperty(f) diff --git a/lib/cache/maybe-github.js b/lib/cache/maybe-github.js index d3f54eb12..fd04eadc8 100644 --- a/lib/cache/maybe-github.js +++ b/lib/cache/maybe-github.js @@ -1,10 +1,15 @@ 'use strict'; -var log = require("npmlog") - , url = require("url") +var url = require("url") + , assert = require("assert") + , log = require("npmlog") , addRemoteGit = require("./add-remote-git.js") -module.exports = function maybeGithub (p, name, er, cb) { +module.exports = function maybeGithub (p, er, cb) { + assert(typeof p === "string", "must pass package name") + assert(er instanceof Error, "must include error") + assert(typeof cb === "function", "must pass callback") + var u = "git://github.com/" + p , up = url.parse(u) log.info("maybeGithub", "Attempting %s from %s", p, u) diff --git a/test/tap/maybe-github.js b/test/tap/maybe-github.js new file mode 100644 index 000000000..8b7105e6e --- /dev/null +++ b/test/tap/maybe-github.js @@ -0,0 +1,76 @@ +require("../common-tap.js") +var test = require("tap").test +var npm = require("../../lib/npm.js") + +// this is the narrowest way to replace a function in the module cache +var found = true +var remoteGitPath = require.resolve('../../lib/cache/add-remote-git.js') +require("module")._cache[remoteGitPath] = { + id: remoteGitPath, + exports: function stub(_, error, __, cb) { + if (found) { + cb(null, {}) + } + else { + cb(error) + } + } +} + +// only load maybeGithub now, so it gets the stub from cache +var maybeGithub = require("../../lib/cache/maybe-github.js") + +test("should throw with no parameters", function (t) { + t.plan(1) + + t.throws(function () { + maybeGithub(); + }, "throws when called without parameters") +}) + +test("should throw with wrong parameter types", function (t) { + t.plan(3) + + t.throws(function () { + maybeGithub({}, new Error(), function () {}) + }, "expects only a package name") + + t.throws(function () { + maybeGithub("npm/xxx-noexist", null, function () {}) + }, "expects to be called after a previous check already failed") + + t.throws(function () { + maybeGithub("npm/xxx-noexist", new Error(), "ham") + }, "is always async") +}) + +test("should find an existing package on Github", function (t) { + found = true + npm.load({}, function (error) { + t.notOk(error, "bootstrapping succeeds") + t.doesNotThrow(function () { + maybeGithub("npm/npm", new Error("not on filesystem"), function (error, data) { + t.notOk(error, "no issues in looking things up") + t.ok(data, "received metadata from Github") + t.end() + }) + }) + }) +}) + +test("shouldn't find a nonexistent package on Github", function (t) { + found = false + npm.load({}, function () { + t.doesNotThrow(function () { + maybeGithub("npm/xxx-noexist", new Error("not on filesystem"), function (error, data) { + t.equal( + error.message, + "not on filesystem", + "passed through original error message" + ) + t.notOk(data, "didn't pass any metadata") + t.end() + }) + }) + }) +}) |