fix run unchanged links lifecycle scripts

Users expect workspaces (or regular linked deps) to run their prepare
lifecycle script on every reify, since it's often used as some form of
compilation step and should run even though that node hasn't changed.

Fixes: npm/cli#2900

PR-URL: #290
Credit: @ruyadorno
Close: #290
Reviewed-by: @isaacs, @nlf

lib/arborist/build-ideal-tree.js

@@ -914,7 +914,7 @@ 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)
+      if (to && to.isLink && to.target)
         this[_linkNodes].add(to)
     }
 

lib/arborist/rebuild.js

@@ -169,7 +169,7 @@ module.exports = cls => class Builder extends cls {
     const queue = [...set].sort(sortNodes)
 
     for (const node of queue) {
-      const { package: { bin, scripts = {} } } = node
+      const { package: { bin, scripts = {} } } = node.target || node
       const { preinstall, install, postinstall, prepare } = scripts
       const tests = { bin, preinstall, install, postinstall, prepare }
       for (const [key, has] of Object.entries(tests)) {
@@ -202,7 +202,7 @@ module.exports = cls => class Builder extends cls {
         !(meta.originalLockfileVersion >= 2)
     }
 
-    const { package: pkg, hasInstallScript } = node
+    const { package: pkg, hasInstallScript } = node.target || node
     const { gypfile, bin, scripts = {} } = pkg
 
     const { preinstall, install, postinstall, prepare } = scripts

lib/arborist/reify.js

@@ -404,8 +404,7 @@ module.exports = cls => class Reifier extends cls {
       return
 
     process.emit('time', 'reify:trashOmits')
-    // node.parent is checked to make sure this is a node that's in the tree, and
-    // not the parent-less top level nodes
+
     const filter = node =>
       node.top.isProjectRoot &&
         (node.peer && this[_omitPeer] ||
@@ -887,6 +886,18 @@ module.exports = cls => class Reifier extends cls {
       filter: diff => diff.action === 'ADD' || diff.action === 'CHANGE',
     })
 
+    // pick up link nodes from the unchanged list as we want to run their
+    // scripts in every install despite of having a diff status change
+    for (const node of this.diff.unchanged) {
+      const tree = node.root.target || node.root
+
+      // skip links that only live within node_modules as they are most
+      // likely managed by packages we installed, we only want to rebuild
+      // unchanged links we directly manage
+      if (node.isLink && node.target.fsTop === tree)
+        nodes.push(node)
+    }
+
     return this.rebuild({ nodes, handleOptionalFailure: true })
       .then(() => process.emit('timeEnd', 'reify:build'))
   }

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

@@ -31821,6 +31821,53 @@ ArboristNode {
 }
 `
 
+exports[`test/arborist/reify.js TAP running lifecycle scripts of unchanged link nodes on reify > result 1`] = `
+ArboristNode {
+  "children": Map {
+    "a" => ArboristLink {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "a",
+          "spec": "file:./a",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/a",
+      "name": "a",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-running-lifecycle-scripts-of-unchanged-link-nodes-on-reify/node_modules/a",
+      "realpath": "{CWD}/test/arborist/tap-testdir-reify-running-lifecycle-scripts-of-unchanged-link-nodes-on-reify/a",
+      "resolved": "file:../a",
+      "target": ArboristNode {
+        "location": "a",
+      },
+      "version": "1.0.0",
+    },
+  },
+  "edgesOut": Map {
+    "a" => EdgeOut {
+      "name": "a",
+      "spec": "file:./a",
+      "to": "node_modules/a",
+      "type": "prod",
+    },
+  },
+  "fsChildren": Set {
+    ArboristNode {
+      "location": "a",
+      "name": "a",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-running-lifecycle-scripts-of-unchanged-link-nodes-on-reify/a",
+      "version": "1.0.0",
+    },
+  },
+  "location": "",
+  "name": "tap-testdir-reify-running-lifecycle-scripts-of-unchanged-link-nodes-on-reify",
+  "packageName": "link-dep-lifecycle-scripts",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-running-lifecycle-scripts-of-unchanged-link-nodes-on-reify",
+  "version": "1.0.0",
+}
+`
+
 exports[`test/arborist/reify.js TAP save complete lockfile on update-all > should have abbrev 1.0.4 1`] = `
 {
   "name": "save-package-lock-after-update-test",

test/arborist/reify.js

@@ -1723,6 +1723,14 @@ console.log('ok 1 - this is fine')
   }), 'test result')
 })
 
+t.test('running lifecycle scripts of unchanged link nodes on reify', async t => {
+  const path = fixture(t, 'link-dep-lifecycle-scripts')
+  t.matchSnapshot(await printReified(path), 'result')
+
+  t.ok(fs.lstatSync(resolve(path, 'a/a-prepare')).isFile(),
+    'should run prepare lifecycle scripts for links directly linked to the tree')
+})
+
 t.test('save-prod, with optional', async t => {
   const path = t.testdir({
     'package.json': JSON.stringify({

test/fixtures/link-dep-lifecycle-scripts/a/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "a",
+  "version": "1.0.0",
+  "scripts": {
+    "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\""
+  }
+}

test/fixtures/link-dep-lifecycle-scripts/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "link-dep-lifecycle-scripts",
+  "version": "1.0.0",
+  "dependencies": {
+    "a": "file:./a"
+  }
+}

test/fixtures/reify-cases/link-dep-lifecycle-scripts.js

@@ -0,0 +1,71 @@
+// generated from test/fixtures/link-dep-lifecycle-scripts
+module.exports = t => {
+  const path = t.testdir({
+  "a": {
+    "package.json": JSON.stringify({
+      "name": "a",
+      "version": "1.0.0",
+      "scripts": {
+        "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\""
+      }
+    })
+  },
+  "node_modules": {
+    ".package-lock.json": JSON.stringify({
+      "name": "link-dep-lifecycle-scripts",
+      "version": "1.0.0",
+      "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": "link-dep-lifecycle-scripts",
+    "version": "1.0.0",
+    "lockfileVersion": 2,
+    "requires": true,
+    "packages": {
+      "": {
+        "version": "1.0.0",
+        "dependencies": {
+          "a": "file:./a"
+        }
+      },
+      "a": {
+        "version": "1.0.0"
+      },
+      "node_modules/a": {
+        "resolved": "a",
+        "link": true
+      }
+    },
+    "dependencies": {
+      "a": {
+        "version": "file:a"
+      }
+    }
+  }),
+  "package.json": JSON.stringify({
+    "name": "link-dep-lifecycle-scripts",
+    "version": "1.0.0",
+    "dependencies": {
+      "a": "file:./a"
+    }
+  })
+})
+  const {utimesSync} = require('fs')
+  const n = Date.now() + 10000
+  const {resolve} = require('path')
+
+  utimesSync(resolve(path, "node_modules/.package-lock.json"), new Date(n), new Date(n))
+  return path
+}