Refactor to be promise-based

This commit changes no functionality, except that a Promise is returned
by the read-package-tree function.  (A supplied callback is attached to
the returned promise if provided.)

Test coverage is brought up to 100%, and some code paths have been
slightly optimized, but the excessive level of realpath/lstat calls
remains for now.

package.json

@@ -7,9 +7,6 @@
     "test": "test"
   },
   "dependencies": {
-    "debuglog": "^1.0.1",
-    "dezalgo": "^1.0.0",
-    "once": "^1.3.0",
     "read-package-json": "^2.0.0",
     "readdir-scoped-modules": "^1.0.0"
   },
@@ -35,5 +32,8 @@
   "homepage": "https://github.com/npm/read-package-tree",
   "files": [
     "rpt.js"
-  ]
+  ],
+  "tap": {
+    "100": true
+  }
 }

rpt.js

@@ -1,249 +1,116 @@
-var fs = require('fs')
-var rpj = require('read-package-json')
-var path = require('path')
-var dz = require('dezalgo')
-var once = require('once')
-var readdir = require('readdir-scoped-modules')
-var debug = require('debuglog')('rpt')
-
-function asyncForEach (items, todo, done) {
-  var remaining = items.length
-  if (remaining === 0) return done()
-  var seenErr
-  items.forEach(function (item) {
-    todo(item, handleComplete)
-  })
-  function handleComplete (err) {
-    if (seenErr) return
-    if (err) {
-      seenErr = true
-      return done(err)
-    }
-    if (--remaining === 0) done()
-  }
-}
-
-function dpath (p) {
-  if (!p) return ''
-  if (p.indexOf(process.cwd()) === 0) {
-    p = p.substr(process.cwd().length + 1)
-  }
-  return p
-}
-
-module.exports = rpt
-
-rpt.Node = Node
-rpt.Link = Link
-
-var ID = 0
-function Node (pkg, logical, physical, er, cache, fromLink) {
-  if (!(this instanceof Node)) {
-    return new Node(pkg, logical, physical, er, cache)
-  }
-
-  var node = cache[physical] || this
-  if (fromLink && cache[physical]) return cache[physical]
-
-  debug(node.constructor.name, dpath(physical), pkg && pkg._id)
-
-  const parent = path.basename(path.dirname(logical))
-  if (parent[0] === '@') {
-    node.name = parent + '/' + path.basename(logical)
-  } else {
-    node.name = path.basename(logical)
+const fs = require('fs')
+const { promisify } = require('util')
+const realpath = promisify(fs.realpath)
+const { basename, dirname, join } = require('path')
+const rpj = promisify(require('read-package-json'))
+const readdir = promisify(require('readdir-scoped-modules'))
+
+let ID = 0
+class Node {
+  constructor (pkg, logical, physical, er, cache) {
+    // should be impossible.
+    /* istanbul ignore next */
+    if (cache.get(physical))
+      throw new Error('re-creating already instantiated node')
+
+    cache.set(physical, this)
+
+    const parent = basename(dirname(logical))
+    if (parent.charAt(0) === '@')
+      this.name = `${parent}/${basename(logical)}`
+    else
+      this.name = basename(logical)
+    this.path = logical
+    this.realpath = physical
+    this.error = er
+    this.id = ID++
+    this.package = pkg || {}
+    this.parent = null
+    this.isLink = false
+    this.children = []
   }
-  node.path = logical
-  node.realpath = physical
-  node.error = er
-  if (!cache[physical]) {
-    node.id = ID++
-    node.package = pkg || {}
-    node.parent = null
-    node.isLink = false
-    node.children = []
-  }
-  return cache[physical] = node
 }
 
-Node.prototype.package = null
-Node.prototype.path = ''
-Node.prototype.realpath = ''
-Node.prototype.children = null
-Node.prototype.error = null
-
-function Link (pkg, logical, physical, realpath, er, cache) {
-  if (cache[physical]) return cache[physical]
-
-  if (!(this instanceof Link)) {
-    return new Link(pkg, logical, physical, realpath, er, cache)
+class Link extends Node {
+  constructor (pkg, logical, physical, realpath, er, cache) {
+    super(pkg, logical, physical, er, cache)
+    const cachedTarget = cache.get(realpath)
+    this.target = cachedTarget || new Node(pkg, logical, realpath, er, cache)
+    this.realpath = realpath
+    this.isLink = true
+    this.children = this.target.children
+    this.error = er
   }
-
-  cache[physical] = this
-
-  debug(this.constructor.name, dpath(physical), pkg && pkg._id)
-
-  const parent = path.basename(path.dirname(logical))
-  if (parent[0] === '@') {
-    this.name = parent + '/' + path.basename(logical)
-  } else {
-    this.name = path.basename(logical)
-  }
-  this.id = ID++
-  this.path = logical
-  this.realpath = realpath
-  this.package = pkg || {}
-  this.parent = null
-  this.target = new Node(this.package, logical, realpath, er, cache, true)
-  this.isLink = true
-  this.children = this.target.children
-  this.error = er
 }
 
-Link.prototype = Object.create(Node.prototype, {
-  constructor: { value: Link }
+const loadNode = (logical, physical, cache) => new Promise((res, rej) => {
+  res(cache.get(physical) || realpath(physical)
+    .then(real =>
+      rpj(join(real, 'package.json'))
+        .then(pkg => [real, pkg, null], er => [real, null, er])
+        .then(([real, pkg, er]) =>
+          physical === real ? new Node(pkg, logical, physical, er, cache)
+          : new Link(pkg, logical, physical, real, er, cache)
+        ),
+      // if the realpath fails, don't bother with the rest
+      er => new Node(null, logical, physical, er, cache))
+    )
 })
-Link.prototype.target = null
-Link.prototype.realpath = ''
-
-function loadNode (logical, physical, cache, cb) {
-  debug('loadNode', dpath(logical))
-  return fs.realpath(physical, thenReadPackageJson)
-
-  var realpath
-  function thenReadPackageJson (er, real) {
-    if (er) {
-      var node = new Node(null, logical, physical, er, cache)
-      return cb(null, node)
-    }
-    debug('realpath l=%j p=%j real=%j', dpath(logical), dpath(physical), dpath(real))
-    var pj = path.join(real, 'package.json')
-    realpath = real
-    return rpj(pj, thenCreateNode)
-  }
-  function thenCreateNode (er, pkg) {
-    pkg = pkg || null
-    var node
-    if (physical === realpath) {
-      node = new Node(pkg, logical, physical, er, cache)
-    } else {
-      node = new Link(pkg, logical, physical, realpath, er, cache)
-    }
-
-    cb(null, node)
-  }
-}
 
-function loadChildren (node, cache, filterWith, cb) {
-  debug('loadChildren', dpath(node.path))
-  // needed 'cause we process all kids async-like and errors
-  // short circuit, so we have to be sure that after an error
-  // the cbs from other kids don't result in calling cb a second
-  // (or more) time.
-  cb = once(cb)
-  var nm = path.join(node.path, 'node_modules')
-  var rm
-  return fs.realpath(path.join(node.path, 'node_modules'), thenReaddir)
-
-  function thenReaddir (er, real_nm) {
-    if (er) return cb(null, node)
-    rm = real_nm
-    readdir(nm, thenLoadKids)
-  }
-
-  function thenLoadKids (er, kids) {
-    // If there are no children, that's fine, just return
-    if (er) return cb(null, node)
-
-    kids = kids.filter(function (kid) {
-      return kid[0] !== '.' && (!filterWith || filterWith(node, kid))
+const loadChildren = (node, cache, filterWith) => {
+  const nm = join(node.path, 'node_modules')
+  return realpath(nm)
+    .then(rm => readdir(rm).then(kids => [rm, kids]))
+    .then(([rm, kids]) => Promise.all(
+      kids.filter(kid =>
+        kid.charAt(0) !== '.' && (!filterWith || filterWith(node, kid)))
+      .map(kid => loadNode(join(nm, kid), join(rm, kid), cache)))
+    ).then(kidNodes => {
+      kidNodes.forEach(k => k.parent = node)
+      node.children = kidNodes.sort((a, b) =>
+        (a.package.name ? a.package.name.toLowerCase() : a.path)
+        .localeCompare(
+          (b.package.name ? b.package.name.toLowerCase() : b.path)
+        ))
+      return node
     })
-
-    asyncForEach(kids, thenLoadNode, thenSortChildren)
-  }
-  function thenLoadNode (kid, done) {
-    var kidPath = path.join(nm, kid)
-    var kidRealPath = path.join(rm, kid)
-    loadNode(kidPath, kidRealPath, cache, andAddNode(done))
-  }
-  function andAddNode (done) {
-    return function (er, kid) {
-      if (er) return done(er)
-      node.children.push(kid)
-      kid.parent = node
-      done()
-    }
-  }
-  function thenSortChildren (er) {
-    sortChildren(node)
-    cb(er, node)
-  }
-}
-
-function sortChildren (node) {
-  node.children = node.children.sort(function (a, b) {
-    a = a.package.name ? a.package.name.toLowerCase() : a.path
-    b = b.package.name ? b.package.name.toLowerCase() : b.path
-    return a > b ? 1 : -1
-  })
+    .catch(() => node)
 }
 
-function loadTree (node, did, cache, filterWith, cb) {
-  debug('loadTree', dpath(node.path), !!cache[node.path])
-
-  if (did[node.realpath]) {
-    return dz(cb)(null, node)
-  }
-
-  did[node.realpath] = true
-
-  // needed 'cause we process all kids async-like and errors
-  // short circuit, so we have to be sure that after an error
-  // the cbs from other kids don't result in calling cb a second
-  // (or more) time.
-  cb = once(cb)
-  return loadChildren(node, cache, filterWith, thenProcessChildren)
+const loadTree = (node, did, cache, filterWith) => {
+  // impossible except in pathological ELOOP cases
+  /* istanbul ignore next */
+  if (did.has(node.realpath))
+    return Promise.resolve(node)
 
-  function thenProcessChildren (er, node) {
-    if (er) return cb(er)
+  did.add(node.realpath)
 
-    var kids = node.children.filter(function (kid) {
-      return !did[kid.realpath]
-    })
-
-    return asyncForEach(kids, loadTreeForKid, cb)
-  }
-  function loadTreeForKid (kid, done) {
-    loadTree(kid, did, cache, filterWith, done)
-  }
+  return loadChildren(node, cache, filterWith)
+    .then(node => Promise.all(
+      node.children
+        .filter(kid => !did.has(kid.realpath))
+        .map(kid => loadTree(kid, did, cache, filterWith))
+    )).then(() => node)
 }
 
-function rpt (root, filterWith, cb) {
-  if (!cb) {
+// XXX Drop filterWith and/or cb in next semver major bump
+const rpt = (root, filterWith, cb) => {
+  if (!cb && typeof filterWith === 'function') {
     cb = filterWith
     filterWith = null
   }
-  var cache = Object.create(null)
-  var topErr
-  var tree
-  return fs.realpath(root, thenLoadNode)
 
-  function thenLoadNode (er, realRoot) {
-    if (er) return cb(er)
-    debug('rpt', dpath(realRoot))
-    loadNode(root, realRoot, cache, thenLoadTree)
-  }
-  function thenLoadTree(er, node) {
-    // even if there's an error, it's fine, as long as we got a node
-    if (node) {
-      topErr = er
-      tree = node
-      loadTree(node, {}, cache, filterWith, thenHandleErrors)
-    } else {
-      cb(er)
-    }
-  }
-  function thenHandleErrors (er) {
-    cb(topErr && topErr.code !== 'ENOENT' ? topErr : er, tree)
-  }
+  const cache = new Map()
+  const p = realpath(root)
+    .then(realRoot => loadNode(root, realRoot, cache))
+    .then(node => loadTree(node, new Set(), cache, filterWith))
+
+  if (typeof cb === 'function')
+    p.then(tree => cb(null, tree), cb)
+
+  return p
 }
+
+rpt.Node = Node
+rpt.Link = Link
+module.exports = rpt

tap-snapshots/test-basic.js-TAP.test.js

@@ -19,6 +19,13 @@ root@1.2.3 test/fixtures/deeproot/root
 └── foo@1.2.3 test/fixtures/deeproot/root/node_modules/foo
 `
 
+exports[`test/basic.js TAP filterWith > must match snapshot 1`] = `
+root@1.2.3 test/fixtures/root
+├── @scope/x@1.2.3 test/fixtures/root/node_modules/@scope/x
+├── @scope/y@1.2.3 test/fixtures/root/node_modules/@scope/y
+└── foo@1.2.3 test/fixtures/root/node_modules/foo
+`
+
 exports[`test/basic.js TAP linkedroot > linkedroot tree 1`] = `
 root@1.2.3 test/fixtures/linkedroot
 ├─┬ @scope/x@1.2.3 test/fixtures/linkedroot/node_modules/@scope/x