fix(arborist): link deps lifecycle scripts (#4875)

- Fixes running proper lifecycle scripts for linked deps and
  workspaces.
- Added test to validate lifecycle scripts don't run twice
  for linked deps
- Tweaked "reify workspaces bin files" test to also validate
  proper lifecycle scripts ran before check for linked bins.
- Tweaked reify test running lifecycle scripts of unchanged link
  nodes to also validate that the install lifecycle scripts are
  also called.

Fixes: #4277
Fixes: #4552
Fixes: npm/statusboard#439
Relates to: #2905

Author: ruyadorno

workspaces/arborist/lib/arborist/rebuild.js

@@ -21,6 +21,8 @@ const sortNodes = (a, b) =>
 
 const _workspaces = Symbol.for('workspaces')
 const _build = Symbol('build')
+const _loadDefaultNodes = Symbol('loadDefaultNodes')
+const _retrieveNodesByType = Symbol('retrieveNodesByType')
 const _resetQueues = Symbol('resetQueues')
 const _rebuildBundle = Symbol('rebuildBundle')
 const _ignoreScripts = Symbol('ignoreScripts')
@@ -39,6 +41,7 @@ const _includeWorkspaceRoot = Symbol.for('includeWorkspaceRoot')
 const _workspacesEnabled = Symbol.for('workspacesEnabled')
 
 const _force = Symbol.for('force')
+const _global = Symbol.for('global')
 
 // defined by reify mixin
 const _handleOptionalFailure = Symbol.for('handleOptionalFailure')
@@ -75,51 +78,83 @@ module.exports = cls => class Builder extends cls {
     // running JUST a rebuild, we treat optional failures as real fails
     this[_doHandleOptionalFailure] = handleOptionalFailure
 
-    // if we don't have a set of nodes, then just rebuild
-    // the actual tree on disk.
     if (!nodes) {
-      const tree = await this.loadActual()
-      let filterSet
-      if (!this[_workspacesEnabled]) {
-        filterSet = this.excludeWorkspacesDependencySet(tree)
-        nodes = tree.inventory.filter(node =>
-          filterSet.has(node) || node.isProjectRoot
-        )
-      } else if (this[_workspaces] && this[_workspaces].length) {
-        filterSet = this.workspaceDependencySet(
-          tree,
-          this[_workspaces],
-          this[_includeWorkspaceRoot]
-        )
-        nodes = tree.inventory.filter(node => filterSet.has(node))
-      } else {
-        nodes = tree.inventory.values()
-      }
+      nodes = await this[_loadDefaultNodes]()
     }
 
     // separates links nodes so that it can run
     // prepare scripts and link bins in the expected order
     process.emit('time', 'build')
+
+    const {
+      depNodes,
+      linkNodes,
+    } = this[_retrieveNodesByType](nodes)
+
+    // build regular deps
+    await this[_build](depNodes, {})
+
+    // build link deps
+    if (linkNodes.size) {
+      this[_resetQueues]()
+      await this[_build](linkNodes, { type: 'links' })
+    }
+
+    process.emit('timeEnd', 'build')
+  }
+
+  // if we don't have a set of nodes, then just rebuild
+  // the actual tree on disk.
+  async [_loadDefaultNodes] () {
+    let nodes
+    const tree = await this.loadActual()
+    let filterSet
+    if (!this[_workspacesEnabled]) {
+      filterSet = this.excludeWorkspacesDependencySet(tree)
+      nodes = tree.inventory.filter(node =>
+        filterSet.has(node) || node.isProjectRoot
+      )
+    } else if (this[_workspaces] && this[_workspaces].length) {
+      filterSet = this.workspaceDependencySet(
+        tree,
+        this[_workspaces],
+        this[_includeWorkspaceRoot]
+      )
+      nodes = tree.inventory.filter(node => filterSet.has(node))
+    } else {
+      nodes = tree.inventory.values()
+    }
+    return nodes
+  }
+
+  [_retrieveNodesByType] (nodes) {
     const depNodes = new Set()
     const linkNodes = new Set()
+
     for (const node of nodes) {
-      // we skip the target nodes to that workspace in order to make sure
-      // we only run lifecycle scripts / place bin links once per workspace
       if (node.isLink) {
         linkNodes.add(node)
       } else {
         depNodes.add(node)
       }
     }
 
-    await this[_build](depNodes, {})
-
-    if (linkNodes.size) {
-      this[_resetQueues]()
-      await this[_build](linkNodes, { type: 'links' })
+    // deduplicates link nodes and their targets, avoids
+    // calling lifecycle scripts twice when running `npm rebuild`
+    // ref: https://github.com/npm/cli/issues/2905
+    //
+    // we avoid doing so if global=true since `bin-links` relies
+    // on having the target nodes available in global mode.
+    if (!this[_global]) {
+      for (const node of linkNodes) {
+        depNodes.delete(node.target)
+      }
     }
 
-    process.emit('timeEnd', 'build')
+    return {
+      depNodes,
+      linkNodes,
+    }
   }
 
   [_resetQueues] () {
@@ -136,24 +171,22 @@ module.exports = cls => class Builder extends cls {
     process.emit('time', `build:${type}`)
 
     await this[_buildQueues](nodes)
+
+    if (!this[_ignoreScripts]) {
+      await this[_runScripts]('preinstall')
+    }
+
     // links should run prepare scripts and only link bins after that
-    if (type !== 'links') {
-      if (!this[_ignoreScripts]) {
-        await this[_runScripts]('preinstall')
-      }
-      if (this[_binLinks]) {
-        await this[_linkAllBins]()
-      }
-      if (!this[_ignoreScripts]) {
-        await this[_runScripts]('install')
-        await this[_runScripts]('postinstall')
-      }
-    } else {
+    if (type === 'links') {
       await this[_runScripts]('prepare')
+    }
+    if (this[_binLinks]) {
+      await this[_linkAllBins]()
+    }
 
-      if (this[_binLinks]) {
-        await this[_linkAllBins]()
-      }
+    if (!this[_ignoreScripts]) {
+      await this[_runScripts]('install')
+      await this[_runScripts]('postinstall')
     }
 
     process.emit('timeEnd', `build:${type}`)

workspaces/arborist/lib/arborist/reify.js

@@ -1105,7 +1105,8 @@ module.exports = cls => class Reifier extends cls {
       // 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) {
+      const linkedFromRoot = node.parent === tree || node.target.fsTop === tree
+      if (node.isLink && linkedFromRoot) {
         nodes.push(node)
       }
     }

workspaces/arborist/test/arborist/rebuild.js

@@ -459,6 +459,51 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => {
   await arb.rebuild()
 })
 
+// ref: https://github.com/npm/cli/issues/2905
+t.test('do not run lifecycle scripts of linked deps twice', async t => {
+  const testdir = t.testdir({
+    project: {
+      'package.json': JSON.stringify({
+        name: 'my-project',
+        version: '1.0.0',
+        dependencies: {
+          foo: 'file:../foo',
+        },
+      }),
+      node_modules: {
+        foo: t.fixture('symlink', '../../foo'),
+      },
+    },
+    foo: {
+      'package.json': JSON.stringify({
+        name: 'foo',
+        version: '1.0.0',
+        scripts: {
+          postinstall: 'echo "ok"',
+        },
+      }),
+    },
+  })
+
+  const path = resolve(testdir, 'project')
+  const RUNS = []
+  const Arborist = t.mock('../../lib/arborist/index.js', {
+    '@npmcli/run-script': opts => {
+      RUNS.push(opts)
+      return require('@npmcli/run-script')(opts)
+    },
+  })
+  const arb = new Arborist({ path, registry })
+  await arb.rebuild()
+  t.equal(RUNS.length, 1, 'should run postinstall script only once')
+  t.match(RUNS, [
+    {
+      event: 'postinstall',
+      pkg: { name: 'foo' },
+    },
+  ])
+})
+
 t.test('workspaces', async t => {
   const path = t.testdir({
     'package.json': JSON.stringify({

workspaces/arborist/test/arborist/reify.js

@@ -1770,6 +1770,8 @@ t.test('running lifecycle scripts of unchanged link nodes on reify', async t =>
 
   t.ok(fs.lstatSync(resolve(path, 'a/a-prepare')).isFile(),
     'should run prepare lifecycle scripts for links directly linked to the tree')
+  t.ok(fs.lstatSync(resolve(path, 'a/a-post-install')).isFile(),
+    'should run postinstall lifecycle scripts for links directly linked to the tree')
 })
 
 t.test('save-prod, with optional', async t => {

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

@@ -2,6 +2,7 @@
   "name": "a",
   "version": "1.0.0",
   "scripts": {
-    "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\""
+    "prepare": "node -e \"require('fs').writeFileSync('a-prepare', '')\"",
+    "postinstall": "node -e \"require('fs').writeFileSync('a-post-install', '')\""
   }
 }

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

@@ -6,7 +6,8 @@ module.exports = t => {
       "name": "a",
       "version": "1.0.0",
       "scripts": {
-        "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\""
+        "prepare": "node -e \"require('fs').writeFileSync('a-prepare', '')\"",
+        "postinstall": "node -e \"require('fs').writeFileSync('a-post-install', '')\""
       }
     })
   },

workspaces/arborist/test/fixtures/reify-cases/workspaces-link-bin.js

@@ -25,13 +25,16 @@ module.exports = t => {
       })
     },
     "b": {
-      "file.js": "",
+      "create-file.js": "require('fs').writeFileSync('file.js', '')\n",
       "package.json": JSON.stringify({
         "name": "b",
         "version": "1.0.0",
         "files": [
           "file.js"
         ],
+        "scripts": {
+          "preinstall": "node create-file.js"
+        },
         "bin": "file.js"
       })
     }

workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/create-file.js

@@ -0,0 +1 @@
+require('fs').writeFileSync('file.js', '')

workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/file.js

@@ -2,5 +2,8 @@
   "name": "b",
   "version": "1.0.0",
   "files": ["file.js"],
+  "scripts": {
+    "preinstall": "node create-file.js"
+  },
   "bin": "file.js"
 }