Handle link deps properly in ideal/virtual/reify

Fix: #43

This fixes the bug identified by npm/cli#750

Link deps that point to a location underneath the root package are fully
resolved and reified.  Links external to the root path are left alone,
because they exist in a different project and presumably have their own
dep resolution being done separately.

lib/arborist/build-ideal-tree.js

@@ -24,6 +24,9 @@ const KEEP = Symbol('KEEP')
 // Yes, clobber the package that is already here
 const REPLACE = Symbol('REPLACE')
 
+const {resolve} = require('path')
+const relpath = require('../relpath.js')
+
 const _depsSeen = Symbol('depsSeen')
 const _depsQueue = Symbol('depsQueue')
 const _currentDep = Symbol('currentDep')
@@ -39,6 +42,7 @@ const _buildDeps = Symbol('buildDeps')
 const _buildDepStep = Symbol('buildDepStep')
 const _nodeFromEdge = Symbol('nodeFromEdge')
 const _nodeFromSpec = Symbol('nodeFromSpec')
+const _linkFromSpec = Symbol('linkFromSpec')
 const _loadPeerSet = Symbol('loadPeerSet')
 // shared symbols so we can hit them with unit tests
 const _updateNames = Symbol.for('updateNames')
@@ -47,6 +51,7 @@ const _canPlaceDep = Symbol.for('canPlaceDep')
 const _canPlacePeers = Symbol('canPlacePeers')
 const _pruneForReplacement = Symbol('pruneForReplacement')
 const _fixDepFlags = Symbol('fixDepFlags')
+const _resolveLinks = Symbol('resolveLinks')
 const _rootNodeFromPackage = Symbol('rootNodeFromPackage')
 const _addRm = Symbol('addRm')
 const _explicitRequests = Symbol('explicitRequests')
@@ -55,6 +60,7 @@ const _shouldUpdateNode = Symbol('shouldUpdateNode')
 const _resetDepFlags = Symbol('resetDepFlags')
 const _loadFailures = Symbol('loadFailures')
 const _pruneFailedOptional = Symbol('pruneFailedOptional')
+const _linkNodes = Symbol('linkNodes')
 
 // used by Reify mixin
 const _idealTreePrune = Symbol.for('idealTreePrune')
@@ -82,6 +88,7 @@ module.exports = cls => class IdealTreeBuilder extends Tracker(Virtual(Actual(cl
     this[_updateAll] = false
     this[_mutateTree] = false
     this[_loadFailures] = new Set()
+    this[_linkNodes] = new Set()
   }
 
   // public method
@@ -258,7 +265,7 @@ module.exports = cls => class IdealTreeBuilder extends Tracker(Virtual(Actual(cl
     }
 
     if (!this[_depsQueue].length)
-      return
+      return this[_resolveLinks]()
 
     // sort physically shallower deps up to the front of the queue,
     // because they'll affect things deeper in, then alphabetical
@@ -384,7 +391,7 @@ module.exports = cls => class IdealTreeBuilder extends Tracker(Virtual(Actual(cl
     // Don't bother to load the manifest for link deps, because the target
     // might be within another package that doesn't exist yet.
     return spec.type === 'directory'
-      ? Promise.resolve(new Link({ name, parent, realpath: spec.fetchSpec }))
+      ? this[_linkFromSpec](name, spec, parent, edge)
       : pacote.manifest(spec, Object.create(this.options))
         .then(pkg => new Node({ name, pkg, parent }), error => {
           error.requiredBy = edge.from.location || '.'
@@ -401,6 +408,15 @@ module.exports = cls => class IdealTreeBuilder extends Tracker(Virtual(Actual(cl
         })
   }
 
+  [_linkFromSpec] (name, spec, parent, edge) {
+    const realpath = spec.fetchSpec
+    return rpj(realpath + '/package.json').catch(() => ({})).then(pkg => {
+      const link = new Link({ name, parent, realpath, pkg })
+      this[_linkNodes].add(link)
+      return link
+    })
+  }
+
   // load all peer deps and meta-peer deps into the node's parent
   // At the end of this, the node's peer-type outward edges are all
   // resolved, and so are all of theirs, but other dep types are not.
@@ -708,6 +724,51 @@ module.exports = cls => class IdealTreeBuilder extends Tracker(Virtual(Actual(cl
     return ret
   }
 
+  // go through all the links in the this[_linkNodes] set
+  // for each one:
+  // - if outside the root, ignore it, assume it's fine, it's not our problem
+  // - if a node in the tree already, assign the target to that node.
+  // - if a path under an existing node, then assign that as the fsParent,
+  //   and add it to the _depsQueue
+  //
+  // call buildDepStep if anything was added to the queue, otherwise we're done
+  [_resolveLinks] () {
+    for (const link of this[_linkNodes]) {
+      this[_linkNodes].delete(link)
+      const realpath = link.realpath
+      const loc = relpath(this.path, realpath)
+      if (/^\.\.[\\\/]/.test(loc)) {
+        // outside the root, somebody else's problem, ignore it
+        // TODO: deep updates somehow, with a flag
+        continue
+      }
+
+      const fromInv = this.idealTree.inventory.get(loc)
+      if (fromInv && fromInv !== link.target)
+        link.target = fromInv
+
+      if (!link.target.parent && !link.target.fsParent) {
+        // the fsParent MUST be some node in the tree, possibly the root.
+        // find it by walking up.  Note that this is where its deps may
+        // end up being installed, if possible.
+        const parts = loc.split('/')
+        let p = parts.length - 1
+        for (let p = parts.length - 1; p > -1; p--) {
+          const path = parts.slice(0, p).join('/')
+          const node = !path ? this.idealTree : this.idealTree.inventory.get(path)
+          if (node) {
+            link.target.fsParent = node
+            this[_depsQueue].push(link.target)
+            p = -1
+          }
+        }
+      }
+    }
+
+    if (this[_depsQueue].length)
+      return this[_buildDepStep]()
+  }
+
   [_fixDepFlags] () {
     const metaFromDisk = this.idealTree.meta.loadedFromDisk
     // if the options set prune:false, then we don't prune, but we still

lib/arborist/load-virtual.js

@@ -98,7 +98,15 @@ module.exports = cls => class VirtualLoader extends cls {
       const parent = nodes.get(ploc)
       /* istanbul ignore else - impossible unless lockfile damaged/invalid */
       if (parent) {
-        node[ name === node.name ? 'parent' : 'fsParent' ] = parent
+        // if the node location doesn't actually start with node_modules, but
+        // the node name DOES match the folder it's in, like if you have a
+        // link from `node_modules/app` to `./app`, then split won't contain
+        // anything, but the name will still match.  In that case, it is an
+        // fsParent, though, not a parent.
+        const parentType = name === node.name && split.length
+          ? 'parent'
+          : 'fsParent'
+        node[ parentType ] = parent
         // read inBundle from package because 'package' here is
         // actually a v2 lockfile metadata entry.
         if (node.package.inBundle && parent.edgesOut.has(name)) {

lib/arborist/reify.js

@@ -53,6 +53,7 @@ const _loadShrinkwrapsAndUpdateTrees = Symbol.for('loadShrinkwrapsAndUpdateTrees
 const _reifyNode = Symbol.for('reifyNode')
 const _checkEngineAndPlatform = Symbol('checkEngineAndPlatform')
 const _extractOrLink = Symbol('extractOrLink')
+const _symlink = Symbol('symlink')
 const _recheckEngineAndPlatform = Symbol('recheckEngineAndPlatform')
 const _checkEngine = Symbol('checkEngine')
 const _checkPlatform = Symbol('checkPlatform')
@@ -266,14 +267,21 @@ module.exports = cls => class Reifier extends Ideal(cls) {
 
   [_extractOrLink] (node) {
     return node.isLink
-      ? rimraf(node.path).then(() => symlink(node.realpath, node.path, 'dir'))
+      ? rimraf(node.path).then(() => this[_symlink](node))
       : pacote.extract(this[_registryResolved](node.resolved), node.path, {
           ...this.options,
           resolved: node.resolved,
           integrity: node.integrity,
         })
   }
 
+  [_symlink] (node) {
+    const dir = dirname(node.path)
+    const target = node.realpath
+    const rel = relative(dir, target)
+    return symlink(rel, node.path, 'dir')
+  }
+
   [_recheckEngineAndPlatform] (node) {
     // If we're loading from a v1 lockfile, then need to do this again later
     // after reading from the disk.