fix: follow symlinks on build-ideal-tree add

Symlinks were being tracked as invalid when loading actual trees since
their realpath wouldn't match, given that build-ideal-tree was just
preserving the fetchSpec read from npm-package-arg when adding new
packages to the ideal tree.

This change fixes it by properly parsing and following symlinks
realpaths at the moment they're added to the ideal tree.

PR-URL: #129
Credit: @ruyadorno
Close: #129
Reviewed-by: @darcyclarke

lib/add-rm-pkg-deps.js

@@ -54,8 +54,9 @@ const addSingle = ({pkg, spec, saveBundle, saveType, path}) => {
 
   pkg[type] = pkg[type] || {}
   if (rawSpec !== '' || pkg[type][name] === undefined) {
-    // if we're in global mode, file specs are based on cwd, not arb path
-    pkg[type][name] = specType === 'file' || specType === 'directory'
+    // file specs are based on arb path, directories specs
+    // are normalized during build-ideal-tree instead
+    pkg[type][name] = specType === 'file'
       ? `file:${relpath(path, fetchSpec)}`
       : (rawSpec || '*')
   }

lib/arborist/build-ideal-tree.js

@@ -8,6 +8,7 @@ const pickManifest = require('npm-pick-manifest')
 const mapWorkspaces = require('@npmcli/map-workspaces')
 const promiseCallLimit = require('promise-call-limit')
 const getPeerSet = require('../peer-set.js')
+const realpath = require('../../lib/realpath.js')
 
 const fromPath = require('../from-path.js')
 const calcDepFlags = require('../calc-dep-flags.js')
@@ -79,6 +80,10 @@ const _globalStyle = Symbol('globalStyle')
 const _globalRootNode = Symbol('globalRootNode')
 const _isVulnerable = Symbol.for('isVulnerable')
 const _usePackageLock = Symbol.for('usePackageLock')
+const _rpcache = Symbol.for('realpathCache')
+const _stcache = Symbol.for('statCache')
+const _followSymlinkPath = Symbol('followSymlinkPath')
+const _retrieveSpecName = Symbol('retrieveSpecName')
 
 // used for the ERESOLVE error to show the last peer conflict encountered
 const _peerConflict = Symbol('peerConflict')
@@ -130,6 +135,12 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     this[_linkNodes] = new Set()
     this[_manifests] = new Map()
     this[_peerConflict] = null
+
+    // caches for cached realpath calls
+    const cwd = process.cwd()
+    // assume that the cwd is real enough for our purposes
+    this[_rpcache] = this[_rpcache]
+    this[_stcache] = this[_stcache]
   }
 
   get explicitRequests () {
@@ -326,18 +337,11 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     return Promise.all(add.map(s => {
       // in global mode, `npm i foo.tgz` needs to be resolved from
       // the current working dir, NOT /usr/local/lib!
-      const spec = npa(s, this[_global] ? process.cwd() : this.path)
-      // if it's just @'' then we reload whatever's there, or get latest
-      // if it's an explicit tag, we need to install that specific tag version
-      const isTag = spec.rawSpec && spec.type === 'tag'
-      return spec.name && !isTag ? spec : pacote.manifest(spec).then(mani => {
-        // if it's a tag type, then we need to run it down to an actual version
-        if (isTag)
-          return npa(`${mani.name}@${mani.version}`)
-
-        spec.name = mani.name
-        return spec
-      })
+      const currpath = this[_global] ? process.cwd() : this.path
+      const spec = npa(s, currpath)
+
+      return this[_retrieveSpecName](spec)
+        .then(add => this[_followSymlinkPath](add, currpath))
     })).then(add => {
       this[_resolvedAdd] = add
       // now add is a list of spec objects with names.
@@ -355,6 +359,41 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     })
   }
 
+  async [_retrieveSpecName] (spec) {
+    // if it's just @'' then we reload whatever's there, or get latest
+    // if it's an explicit tag, we need to install that specific tag version
+    const isTag = spec.rawSpec && spec.type === 'tag'
+
+    if (spec.name && !isTag)
+      return spec
+
+    const mani = await pacote.manifest(spec)
+    // if it's a tag type, then we need to run it down to an actual version
+    if (isTag)
+      return npa(`${mani.name}@${mani.version}`)
+
+    spec.name = mani.name
+    return spec
+  }
+
+  async [_followSymlinkPath] (spec, currpath) {
+    if (spec.type === 'directory') {
+      const real = await (
+        realpath(spec.fetchSpec, this[_rpcache], this[_stcache])
+          // TODO: create synthetic test case to simulate realpath failure
+          .catch(/* istanbul ignore next */() => null)
+      )
+
+      /* istanbul ignore else - same as the ignored catch above */
+      if (real) {
+        const { name } = spec
+        spec = npa(`file:${relpath(this.path, real)}`, this.path)
+        spec.name = name
+      }
+    }
+    return spec
+  }
+
   // TODO: provide a way to fix bundled deps by exposing metadata about
   // what's in the bundle at each published manifest.  Without that, we
   // can't possibly fix bundled deps without breaking a ton of other stuff,

lib/arborist/load-actual.js

@@ -24,8 +24,8 @@ const _findMissingEdges = Symbol('findMissingEdges')
 const _findFSParents = Symbol('findFSParents')
 
 const _actualTreeLoaded = Symbol('actualTreeLoaded')
-const _rpcache = Symbol('realpathCache')
-const _stcache = Symbol('statCache')
+const _rpcache = Symbol.for('realpathCache')
+const _stcache = Symbol.for('statCache')
 const _topNodes = Symbol('linkTargets')
 const _cache = Symbol('nodeLoadingCache')
 const _loadActual = Symbol('loadActual')

tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js

@@ -223,6 +223,41 @@ Node {
 }
 `
 
+exports[`test/arborist/build-ideal-tree.js TAP add symlink that points to a symlink > should follow symlinks to find final realpath destination 1`] = `
+Node {
+  "children": Map {
+    "a" => Link {
+      "edgesIn": Set {
+        Edge {
+          "from": "",
+          "name": "a",
+          "spec": "file:../linked-pkg",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/a",
+      "name": "a",
+      "resolved": "file:../../linked-pkg",
+      "target": Object {
+        "name": "a",
+        "parent": null,
+      },
+    },
+  },
+  "edgesOut": Map {
+    "a" => Edge {
+      "name": "a",
+      "spec": "file:../linked-pkg",
+      "to": "node_modules/a",
+      "type": "prod",
+    },
+  },
+  "location": "",
+  "name": "my-project",
+  "resolved": null,
+}
+`
+
 exports[`test/arborist/build-ideal-tree.js TAP bad shrinkwrap file > bad shrinkwrap 1`] = `
 Node {
   "children": Map {

test/arborist/build-ideal-tree.js

@@ -1321,6 +1321,42 @@ t.test('workspaces', t => {
   t.end()
 })
 
+t.test('add symlink that points to a symlink', t => {
+  const fixt = t.testdir({
+    'global-prefix': {
+      lib: {
+        node_modules: {
+          a: t.fixture('symlink', '../../../linked-pkg')
+        }
+      }
+    },
+    'linked-pkg': {
+      'package.json': JSON.stringify({
+        name: 'a',
+        version: '1.0.0'
+      })
+    },
+    'my-project': {
+      'package.json': JSON.stringify({})
+    }
+  })
+  const path = resolve(fixt, 'my-project')
+  const arb = new Arborist({
+    path,
+    ...OPT,
+  })
+  return arb.buildIdealTree({
+    add: [
+      'file:../global-prefix/lib/node_modules/a'
+    ]
+  }).then(tree =>
+    t.matchSnapshot(
+      printTree(tree),
+      'should follow symlinks to find final realpath destination'
+    )
+  )
+})
+
 // if we get this wrong, it'll spin forever and use up all the memory
 t.test('pathologically nested dependency cycle', t =>
   t.resolveMatchSnapshot(printIdeal(
@@ -1349,24 +1385,35 @@ t.test('resolve files from cwd in global mode, Arb path in local mode', t => {
 })
 
 t.only('resolve links in global mode', t => {
-  const cwd = process.cwd()
-  t.teardown(() => process.chdir(cwd))
   const path = t.testdir({
-    global: {}
+    global: {},
+    lib: {
+      'my-project': {}
+    },
+    'linked-dep': {
+      'package.json': JSON.stringify({
+        name: 'linked-dep',
+        version: '1.0.0'
+      })
+    }
   })
-  const fixturedir = resolve(fixtures, 'root-bundler')
+  const fixturedir = resolve(path, 'lib', 'my-project')
+
+  const cwd = process.cwd()
+  t.teardown(() => process.chdir(cwd))
   process.chdir(fixturedir)
+
   const arb = new Arborist({
+    ...OPT,
     global: true,
     path: resolve(path, 'global'),
-    ...OPT,
   })
   return arb.buildIdealTree({
-    add: ['file:../sax'],
+    add: ['file:../../linked-dep'],
     global: true,
   }).then(tree => {
-    const resolved = 'file:../../../../fixtures/sax'
-    t.equal(tree.children.get('sax').resolved, resolved)
+    const resolved = 'file:../../linked-dep'
+    t.equal(tree.children.get('linked-dep').resolved, resolved)
   })
 })