rm fetch-package-metadata, refactor bugs/repo/docs

- remove the now-outdated 'fetch-package-metadata' module.
- refactor the `bugs`, `repo`, and `docs` commands for consistency.
- add unit tests for refactored commands and new util module.
- update `browser` config handling to honor `browser = false` in config
  files, along with command line flag.  (previously only cli config was
  honored.)
- slight improvement to `open-url` output when browser not used.

9 files changed, 174 insertions, 271 deletions

diff --git a/lib/bugs.js b/lib/bugs.js
index 10300d1e1..55f81989a 100644
--- a/lib/bugs.js
+++ b/lib/bugs.js
@@ -1,31 +1,49 @@
-module.exports = bugs
-
-var log = require('npmlog')
-var openUrl = require('./utils/open-url')
-var fetchPackageMetadata = require('./fetch-package-metadata.js')
-var usage = require('./utils/usage')
-
-bugs.usage = usage(
-  'bugs',
-  'npm bugs [<pkgname>]'
-)
-
-bugs.completion = function (opts, cb) {
-  // FIXME: there used to be registry completion here, but it stopped making
-  // sense somewhere around 50,000 packages on the registry
-  cb()
-}
+const log = require('npmlog')
+const pacote = require('pacote')
+const { promisify } = require('util')
+const openUrl = promisify(require('./utils/open-url.js'))
+const usageUtil = require('./utils/usage.js')
+const npm = require('./npm.js')
+const hostedFromMani = require('./utils/hosted-git-info-from-manifest.js')
+
+const usage = usageUtil('bugs', 'npm bugs [<pkgname>]')
+const completion = (opts, cb) => cb(null, [])
 
-function bugs (args, cb) {
-  var n = args.length ? args[0] : '.'
-  fetchPackageMetadata(n, '.', {fullMetadata: true}, function (er, d) {
-    if (er) return cb(er)
+const cmd = (args, cb) => bugs(args).then(() => cb()).catch(cb)
 
-    var url = d.bugs && ((typeof d.bugs === 'string') ? d.bugs : d.bugs.url)
-    if (!url) {
-      url = 'https://www.npmjs.org/package/' + d.name
+const bugs = async args => {
+  if (!args || !args.length) {
+    args = ['.']
+  }
+  await Promise.all(args.map(pkg => getBugs(pkg)))
+}
+
+const getBugsUrl = mani => {
+  if (mani.bugs) {
+    if (typeof mani.bugs === 'string') {
+      return mani.bugs
+    }
+    if (typeof mani.bugs === 'object' && mani.bugs.url) {
+      return mani.bugs.url
     }
-    log.silly('bugs', 'url', url)
-    openUrl(url, 'bug list available at the following URL', cb)
-  })
+  }
+
+  // try to get it from the repo, if possible
+  const info = hostedFromMani(mani)
+  if (info) {
+    return info.bugs()
+  }
+
+  // just send them to the website, hopefully that has some info!
+  return  `https://www.npmjs.com/package/${mani.name}`
 }
+
+const getBugs = async pkg => {
+  const opts = { ...npm.flatOptions, fullMetadata: true }
+  const mani = await pacote.manifest(pkg, { fullMetadata: true })
+  const url = getBugsUrl(mani)
+  log.silly('bugs', 'url', url)
+  await openUrl(url, `${mani.name} bug list available at the following URL`)
+}
+
+module.exports = Object.assign(cmd, { usage, completion })
diff --git a/lib/config/defaults.js b/lib/config/defaults.js
index 0ca1472eb..b47adf673 100644
--- a/lib/config/defaults.js
+++ b/lib/config/defaults.js
@@ -269,7 +269,7 @@ exports.types = {
   'auth-type': ['legacy', 'sso', 'saml', 'oauth'],
   'before': [null, Date],
   'bin-links': Boolean,
-  browser: [null, String],
+  browser: [null, Boolean, String],
   ca: [null, String, Array],
   cafile: path,
   cache: path,
diff --git a/lib/docs.js b/lib/docs.js
index 6d67da4e1..4a1dbf7ed 100644
--- a/lib/docs.js
+++ b/lib/docs.js
@@ -1,41 +1,42 @@
-module.exports = docs
-
-var openUrl = require('./utils/open-url')
-var log = require('npmlog')
-var fetchPackageMetadata = require('./fetch-package-metadata.js')
-var usage = require('./utils/usage')
-
-docs.usage = usage(
-  'docs',
-  'npm docs <pkgname>' +
-  '\nnpm docs .'
-)
-docs.completion = function (opts, cb) {
-  // FIXME: there used to be registry completion here, but it stopped making
-  // sense somewhere around 50,000 packages on the registry
-  cb()
+const log = require('npmlog')
+const pacote = require('pacote')
+const { promisify } = require('util')
+const openUrl = promisify(require('./utils/open-url.js'))
+const usageUtil = require('./utils/usage.js')
+const npm = require('./npm.js')
+const hostedFromMani = require('./utils/hosted-git-info-from-manifest.js')
+
+const usage = usageUtil('docs', 'npm docs [<pkgname> [<pkgname> ...]]')
+const completion = (opts, cb) => cb(null, [])
+
+const cmd = (args, cb) => docs(args).then(() => cb()).catch(cb)
+
+const docs = async args => {
+  if (!args || !args.length) {
+    args = ['.']
+  }
+  await Promise.all(args.map(pkg => getDocs(pkg)))
 }
 
-function docs (args, cb) {
-  if (!args || !args.length) args = ['.']
-  var pending = args.length
-  log.silly('docs', args)
-  args.forEach(function (proj) {
-    getDoc(proj, function (err) {
-      if (err) {
-        return cb(err)
-      }
-      --pending || cb()
-    })
-  })
+const getDocsUrl = mani => {
+  if (mani.homepage) {
+    return mani.homepage
+  }
+
+  const info = hostedFromMani(mani)
+  if (info) {
+    return info.docs()
+  }
+
+  return 'https://www.npmjs.com/package/' + mani.name
 }
 
-function getDoc (project, cb) {
-  log.silly('getDoc', project)
-  fetchPackageMetadata(project, '.', {fullMetadata: true}, function (er, d) {
-    if (er) return cb(er)
-    var url = d.homepage
-    if (!url) url = 'https://www.npmjs.org/package/' + d.name
-    return openUrl(url, 'docs available at the following URL', cb)
-  })
+const getDocs = async pkg => {
+  const opts = { ...npm.flatOptions, fullMetadata: true }
+  const mani = await pacote.manifest(pkg, opts)
+  const url = getDocsUrl(mani)
+  log.silly('docs', 'url', url)
+  await openUrl(url, `${mani.name} docs available at the following URL`)
 }
+
+module.exports = Object.assign(cmd, { usage, completion })
diff --git a/lib/doctor/get-latest-npm-version.js b/lib/doctor/get-latest-npm-version.js
index 5a096ab89..83b600396 100644
--- a/lib/doctor/get-latest-npm-version.js
+++ b/lib/doctor/get-latest-npm-version.js
@@ -1,14 +1,19 @@
-var log = require('npmlog')
-var fetchPackageMetadata = require('../fetch-package-metadata')
+const log = require('npmlog')
+const pacote = require('pacote')
+const npm = require('../npm.js')
 
-function getLatestNpmVersion (cb) {
-  var tracker = log.newItem('getLatestNpmVersion', 1)
+const getLatestNpmVersion = async cb => {
+  const tracker = log.newItem('getLatestNpmVersion', 1)
   tracker.info('getLatestNpmVersion', 'Getting npm package information')
-  fetchPackageMetadata('npm@latest', '.', {}, function (err, d) {
-    tracker.finish()
-    if (err) { return cb(err) }
-    cb(null, d.version)
-  })
+  let version = null
+  let error = null
+  try {
+    version = (await pacote.manifest('npm@latest')).version
+  } catch (er) {
+    error = er
+  }
+  tracker.finish()
+  return cb(error, version)
 }
 
 module.exports = getLatestNpmVersion
diff --git a/lib/fetch-package-metadata.js b/lib/fetch-package-metadata.js
deleted file mode 100644
index c4f46f513..000000000
--- a/lib/fetch-package-metadata.js
+++ /dev/null
@@ -1,119 +0,0 @@
-'use strict'
-
-const deprCheck = require('./utils/depr-check')
-const path = require('path')
-const log = require('npmlog')
-const readPackageTree = require('read-package-tree')
-const rimraf = require('rimraf')
-const validate = require('aproba')
-const npa = require('npm-package-arg')
-const npm = require('./npm')
-let npmConfig
-const npmlog = require('npmlog')
-const limit = require('call-limit')
-const tempFilename = require('./utils/temp-filename')
-const pacote = require('pacote')
-const isWindows = require('./utils/is-windows.js')
-
-function andLogAndFinish (spec, tracker, done) {
-  validate('SOF|SZF|OOF|OZF', [spec, tracker, done])
-  return (er, pkg) => {
-    if (er) {
-      log.silly('fetchPackageMetaData', 'error for ' + String(spec), er.message)
-      if (tracker) tracker.finish()
-    }
-    return done(er, pkg)
-  }
-}
-
-const LRUCache = require('lru-cache')
-const CACHE = new LRUCache({
-  max: 300 * 1024 * 1024,
-  length: (p) => p._contentLength
-})
-
-module.exports = limit(fetchPackageMetadata, npm.limit.fetch)
-function fetchPackageMetadata (spec, where, opts, done) {
-  validate('SSOF|SSFZ|OSOF|OSFZ', [spec, where, opts, done])
-
-  if (!done) {
-    done = opts
-    opts = {}
-  }
-  var tracker = opts.tracker
-  const logAndFinish = andLogAndFinish(spec, tracker, done)
-
-  if (typeof spec === 'object') {
-    var dep = spec
-  } else {
-    dep = npa(spec)
-  }
-  if (!isWindows && dep.type === 'directory' && /^[a-zA-Z]:/.test(dep.fetchSpec)) {
-    var err = new Error(`Can't install from windows path on a non-windows system: ${dep.fetchSpec.replace(/[/]/g, '\\')}`)
-    err.code = 'EWINDOWSPATH'
-    return logAndFinish(err)
-  }
-  if (!npmConfig) {
-    npmConfig = require('./config/figgy-config.js')
-  }
-  pacote.manifest(dep, npmConfig({
-    annotate: true,
-    fullMetadata: opts.fullMetadata,
-    log: tracker || npmlog,
-    memoize: CACHE,
-    where: where
-  })).then(
-    (pkg) => logAndFinish(null, deprCheck(pkg)),
-    (err) => {
-      if (dep.type !== 'directory') return logAndFinish(err)
-      if (err.code === 'ENOTDIR') {
-        var enolocal = new Error(`Could not install "${path.relative(process.cwd(), dep.fetchSpec)}" as it is not a directory and is not a file with a name ending in .tgz, .tar.gz or .tar`)
-        enolocal.code = 'ENOLOCAL'
-        if (err.stack) enolocal.stack = err.stack
-        return logAndFinish(enolocal)
-      } else if (err.code === 'ENOPACKAGEJSON') {
-        var enopackage = new Error(`Could not install from "${path.relative(process.cwd(), dep.fetchSpec)}" as it does not contain a package.json file.`)
-        enopackage.code = 'ENOLOCAL'
-        if (err.stack) enopackage.stack = err.stack
-        return logAndFinish(enopackage)
-      } else {
-        return logAndFinish(err)
-      }
-    }
-  )
-}
-
-module.exports.addBundled = addBundled
-function addBundled (pkg, next) {
-  validate('OF', arguments)
-  if (pkg._bundled !== undefined) return next(null, pkg)
-
-  if (!pkg.bundleDependencies && pkg._requested.type !== 'directory') return next(null, pkg)
-  const requested = pkg._requested || npa(pkg._from)
-  if (requested.type === 'directory') {
-    pkg._bundled = null
-    return readPackageTree(pkg._requested.fetchSpec, function (er, tree) {
-      if (tree) pkg._bundled = tree.children
-      return next(null, pkg)
-    })
-  }
-  pkg._bundled = null
-  const target = tempFilename('unpack')
-  if (!npmConfig) {
-    npmConfig = require('./config/figgy-config.js')
-  }
-  const opts = npmConfig({integrity: pkg._integrity})
-  pacote.extract(pkg._resolved || pkg._requested || npa.resolve(pkg.name, pkg.version), target, opts).then(() => {
-    log.silly('addBundled', 'read tarball')
-    readPackageTree(target, (err, tree) => {
-      if (err) { return next(err) }
-      log.silly('cleanup', 'remove extracted module')
-      rimraf(target, function () {
-        if (tree) {
-          pkg._bundled = tree.children
-        }
-        next(null, pkg)
-      })
-    })
-  }, next)
-}
diff --git a/lib/fetch-package-metadata.md b/lib/fetch-package-metadata.md
deleted file mode 100644
index 7b4562341..000000000
--- a/lib/fetch-package-metadata.md
+++ /dev/null
@@ -1,37 +0,0 @@
-fetch-package-metadata
-----------------------
-
-    const fetchPackageMetadata = require("npm/lib/fetch-package-metadata")
-    fetchPackageMetadata(spec, contextdir, callback)
-
-This will get package metadata (and if possible, ONLY package metadata) for
-a specifier as passed to `npm install` et al, eg `npm@next` or `npm@^2.0.3`
-
-## fetchPackageMetadata(*spec*, *contextdir*, *tracker*, *callback*)
-
-* *spec* **string** | **object** -- The package specifier, can be anything npm can
-  understand (see [realize-package-specifier]), or it can be the result from
-  realize-package-specifier or npm-package-arg (for non-local deps).
-
-* *contextdir* **string** -- The directory from which relative paths to
-  local packages should be resolved.
-
-* *tracker* **object** -- **(optional)** An are-we-there-yet tracker group as
-  provided by `npm.log.newGroup()`.
-
-* *callback* **function (er, package)** -- Called when the package information
-  has been loaded. `package` is the object for of the `package.json`
-  matching the requested spec.  In the case of named packages, it comes from
-  the registry and thus may not exactly match what's found in the associated
-  tarball.
-
-[realize-package-specifier]: (https://github.com/npm/realize-package-specifier)
-
-In the case of tarballs and git repos, it will use the cache to download
-them in order to get the package metadata.  For named packages, only the
-metadata is downloaded (eg https://registry.npmjs.org/package).  For local
-directories, the package.json is read directly.  For local tarballs, the
-tarball is streamed in memory and just the package.json is extracted from
-it.  (Due to the nature of tars, having the package.json early in the file
-will result in it being loaded faster– the extractor short-circuits the
-uncompress/untar streams as best as it can.)
diff --git a/lib/repo.js b/lib/repo.js
index b930402ae..455ca9f33 100644
--- a/lib/repo.js
+++ b/lib/repo.js
@@ -1,50 +1,72 @@
-module.exports = repo
+const log = require('npmlog')
+const pacote = require('pacote')
+const { promisify } = require('util')
+const openUrl = promisify(require('./utils/open-url.js'))
+const usageUtil = require('./utils/usage.js')
+const npm = require('./npm.js')
+const hostedFromMani = require('./utils/hosted-git-info-from-manifest.js')
+const { URL } = require('url')
 
-repo.usage = 'npm repo [<pkg>]'
+const usage = usageUtil('repo', 'npm repo [<pkgname> [<pkgname> ...]]')
+const completion = (opts, cb) => cb(null, [])
 
-const openUrl = require('./utils/open-url')
-const hostedGitInfo = require('hosted-git-info')
-const url_ = require('url')
-const fetchPackageMetadata = require('./fetch-package-metadata.js')
+const cmd = (args, cb) => repo(args).then(() => cb()).catch(cb)
 
-repo.completion = function (opts, cb) {
-  // FIXME: there used to be registry completion here, but it stopped making
-  // sense somewhere around 50,000 packages on the registry
-  cb()
+const repo = async args => {
+  if (!args || !args.length) {
+    args = ['.']
+  }
+  await Promise.all(args.map(pkg => getRepo(pkg)))
 }
 
-function repo (args, cb) {
-  const n = args.length ? args[0] : '.'
-  fetchPackageMetadata(n, '.', {fullMetadata: true}, function (er, d) {
-    if (er) return cb(er)
-    getUrlAndOpen(d, cb)
-  })
-}
+const getRepo = async pkg => {
+  const opts = { ...npm.flatOptions, fullMetadata: true }
+  const mani = await pacote.manifest(pkg, opts)
+
+  const r = mani.repository
+  const rurl = !r ? null
+    : typeof r === 'string' ? r
+    : typeof r === 'object' && typeof r.url === 'string' ? r.url
+    : null
+
+  if (!rurl) {
+    throw Object.assign(new Error('no repository'), {
+      pkgid: pkg
+    })
+  }
 
-function getUrlAndOpen (d, cb) {
-  const r = d.repository
-  if (!r) return cb(new Error('no repository'))
-  // XXX remove this when npm@v1.3.10 from node 0.10 is deprecated
-  // from https://github.com/npm/npm-www/issues/418
-  const info = hostedGitInfo.fromUrl(r.url)
-  const url = info ? info.browse() : unknownHostedUrl(r.url)
+  const info = hostedFromMani(mani)
+  const url = info ? info.browse() : unknownHostedUrl(rurl)
 
-  if (!url) return cb(new Error('no repository: could not get url'))
+  if (!url) {
+    throw Object.assign(new Error('no repository: could not get url'), {
+      pkgid: pkg
+    })
+  }
 
-  openUrl(url, 'repository available at the following URL', cb)
+  log.silly('docs', 'url', url)
+  await openUrl(url, `${mani.name} repo available at the following URL`)
 }
 
-function unknownHostedUrl (url) {
+const unknownHostedUrl = url => {
   try {
-    const idx = url.indexOf('@')
-    if (idx !== -1) {
-      url = url.slice(idx + 1).replace(/:([^\d]+)/, '/$1')
+    const {
+      protocol,
+      hostname,
+      pathname
+    } = new URL(url)
+
+    /* istanbul ignore next - URL ctor should prevent this */
+    if (!protocol || !hostname) {
+      return null
     }
-    url = url_.parse(url)
-    const protocol = url.protocol === 'https:'
-      ? 'https:'
-      : 'http:'
-    return protocol + '//' + (url.host || '') +
-      url.path.replace(/\.git$/, '')
-  } catch (e) {}
+
+    const proto = /(git\+)http:$/.test(protocol) ? 'http:' : 'https:'
+    const path = pathname.replace(/\.git$/, '')
+    return `${proto}//${hostname}${path}`
+  } catch (e) {
+    return null
+  }
 }
+
+module.exports = Object.assign(cmd, { usage, completion })
diff --git a/lib/utils/hosted-git-info-from-manifest.js b/lib/utils/hosted-git-info-from-manifest.js
new file mode 100644
index 000000000..ecb7555b1
--- /dev/null
+++ b/lib/utils/hosted-git-info-from-manifest.js
@@ -0,0 +1,14 @@
+// given a manifest, try to get the hosted git info from it based on
+// repository (if a string) or repository.url (if an object)
+// returns null if it's not a valid repo, or not a known hosted repo
+const hostedGitInfo = require('hosted-git-info')
+module.exports = mani => {
+  const r = mani.repository
+  const rurl = !r ? null
+    : typeof r === 'string' ? r
+    : typeof r === 'object' && typeof r.url === 'string' ? r.url
+    : null
+
+  // hgi returns undefined sometimes, but let's always return null here
+  return rurl && hostedGitInfo.fromUrl(rurl.replace(/^git\+/, '')) || null
+}
diff --git a/lib/utils/open-url.js b/lib/utils/open-url.js
index 58227f039..a38a881b5 100644
--- a/lib/utils/open-url.js
+++ b/lib/utils/open-url.js
@@ -22,14 +22,12 @@ module.exports = function open (url, errMsg, cb, browser = npm.config.get('brows
         title: errMsg,
         url
       }, null, 2)
-      : `${errMsg}:\n\n${url}`
+      : `${errMsg}:\n  ${url}\n`
 
     output(alternateMsg)
   }
 
-  const skipBrowser = process.argv.indexOf('--no-browser') > -1
-
-  if (skipBrowser) {
+  if (browser === false) {
     printAlternateMsg()
     return cb()
   }
@@ -38,7 +36,8 @@ module.exports = function open (url, errMsg, cb, browser = npm.config.get('brows
     return cb(new Error('Invalid URL: ' + url))
   }
 
-  opener(url, { command: browser }, (er) => {
+  const command = browser === true ? null : browser
+  opener(url, { command }, (er) => {
     if (er && er.code === 'ENOENT') {
       printAlternateMsg()
       return cb()