fix: process deps for all link nodes

Arborist wasn't checking for any package.json dep changes within its
Link nodes thus preventing users from adding new deps on workspaces.

This changeset fixes it and adds a reify test repro.

Fixes: npm/cli#1984

lib/arborist/build-ideal-tree.js

@@ -790,6 +790,11 @@ This is a one-time fix-up, please be patient...
     }
     await Promise.all(promises)
 
+    for (const { to } of node.edgesOut.values()) {
+      if (to && to.isLink)
+        this[_linkNodes].add(to)
+    }
+
     return this[_buildDepStep]()
   }
 
@@ -1539,9 +1544,14 @@ This is a one-time fix-up, please be patient...
         }
       }
 
-      // didn't find a parent for it, but we're filling in external
-      // link targets, so go ahead and process it.
-      if (this[_follow] && !link.target.parent && !link.target.fsParent) {
+      // didn't find a parent for it or it has not been seen yet
+      // so go ahead and process it.
+      const unseenLink = (link.target.parent || link.target.fsParent)
+        && !this[_depsSeen].has(link.target)
+      if (this[_follow]
+        && !link.target.parent
+        && !link.target.fsParent
+        || unseenLink) {
         this.addTracker('idealTree', link.target.name, link.target.location)
         this[_depsQueue].push(link.target)
       }

lib/arborist/load-virtual.js

@@ -185,29 +185,20 @@ module.exports = cls => class VirtualLoader extends cls {
   // links is the set of metadata, and nodes is the map of non-Link nodes
   // Set the targets to nodes in the set, if we have them (we might not)
   async [resolveLinks] (links, nodes) {
-    // now we've loaded the root, and all real nodes
-    // link up the links
-    const {meta} = this.virtualTree
-    const {loadedFromDisk, originalLockfileVersion} = meta
-    const oldLockfile = loadedFromDisk && !(originalLockfileVersion >= 2)
-
     for (const [location, meta] of links.entries()) {
       const targetPath = resolve(this.path, meta.resolved)
       const targetLoc = relpath(this.path, targetPath)
       const target = nodes.get(targetLoc)
       const link = this[loadLink](location, targetLoc, target, meta)
       nodes.set(location, link)
       nodes.set(targetLoc, link.target)
-      // legacy shrinkwraps do not store all the info we need for the target.
-      // if we're loading from disk, and have a link in place, we need to
-      // look in that actual folder (or at least try to) in order to get
-      // the dependencies of the link target and load it properly.
-      if (oldLockfile) {
-        const pj = link.realpath + '/package.json'
-        const pkg = await rpj(pj).catch(() => null)
-        if (pkg)
-          link.target.package = pkg
-      }
+
+      // we always need to read the package.json for link targets
+      // because they can be changed by the local user
+      const pj = link.realpath + '/package.json'
+      const pkg = await rpj(pj).catch(() => null)
+      if (pkg)
+        link.target.package = pkg
     }
   }
 

tap-snapshots/test-arborist-reify.js-TAP.test.js

@@ -45654,6 +45654,50 @@ Node {
 }
 `
 
+exports[`test/arborist/reify.js TAP workspaces add new workspaces dep > should update package-lock with new added dep 1`] = `
+Object {
+  "dependencies": Object {
+    "a": Object {
+      "requires": Object {
+        "abbrev": "^1.1.1",
+      },
+      "version": "file:a",
+    },
+    "abbrev": Object {
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "lockfileVersion": 2,
+  "name": "workspaces-add-new-dep",
+  "packages": Object {
+    "": Object {
+      "name": "workspaces-add-new-dep",
+      "workspaces": Array [
+        "a",
+      ],
+    },
+    "a": Object {
+      "dependencies": Object {
+        "abbrev": "^1.1.1",
+      },
+      "version": "1.0.0",
+    },
+    "node_modules/a": Object {
+      "link": true,
+      "resolved": "a",
+    },
+    "node_modules/abbrev": Object {
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "requires": true,
+}
+`
+
 exports[`test/arborist/reify.js TAP workspaces reify from an actual loaded workspace env > should not clean up entire nm folder for no reason 1`] = `
 Node {
   "children": Map {

test/arborist/reify.js

@@ -1057,6 +1057,14 @@ t.test('workspaces', t => {
       'should not clean up entire nm folder for no reason'
     ))
 
+  t.test('add new workspaces dep', t => {
+    const path = fixture(t, 'workspaces-add-new-dep')
+    reify(path).then(() => {
+      t.matchSnapshot(require(path + '/package-lock.json'), 'should update package-lock with new added dep')
+      t.end()
+    })
+  })
+
   t.end()
 })
 

test/fixtures/reify-cases/workspaces-add-new-dep.js

@@ -0,0 +1,67 @@
+// generated from test/fixtures/workspaces-add-new-dep
+module.exports = t => {
+  const path = t.testdir({
+  "a": {
+    "package.json": JSON.stringify({
+      "name": "a",
+      "version": "1.0.0",
+      "dependencies": {
+        "abbrev": "^1.1.1"
+      }
+    })
+  },
+  "node_modules": {
+    ".package-lock.json": JSON.stringify({
+      "name": "workspaces-add-new-dep",
+      "lockfileVersion": 2,
+      "requires": true,
+      "packages": {
+        "a": {
+          "version": "1.0.0"
+        },
+        "node_modules/a": {
+          "resolved": "a",
+          "link": true
+        }
+      }
+    }),
+    "a": t.fixture('symlink', "../a")
+  },
+  "package-lock.json": JSON.stringify({
+    "name": "workspaces-add-new-dep",
+    "lockfileVersion": 2,
+    "requires": true,
+    "packages": {
+      "": {
+        "workspaces": [
+          "a"
+        ]
+      },
+      "a": {
+        "version": "1.0.0"
+      },
+      "node_modules/a": {
+        "resolved": "a",
+        "link": true
+      }
+    },
+    "dependencies": {
+      "a": {
+        "version": "file:a"
+      }
+    }
+  }),
+  "package.json": JSON.stringify({
+    "name": "workspaces-add-new-dep",
+    "workspaces": [
+      "a"
+    ]
+  })
+})
+  const {utimesSync} = require('fs')
+  const n = Date.now()
+  const {resolve} = require('path')
+
+  utimesSync(resolve(path, "node_modules/.package-lock.json"), new Date(n), new Date(n))
+  return path
+}

test/fixtures/workspaces-add-new-dep/a/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "a",
+  "version": "1.0.0",
+  "//": "abbrev is a new dep, not yet installed:",
+  "dependencies": {
+    "abbrev": "^1.1.1"
+  }
+}

test/fixtures/workspaces-add-new-dep/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "workspaces-add-new-dep",
+  "workspaces": [
+    "a"
+  ]
+}