feat(lifecycle): run scripts in dep order (#23)

Fixes: #16

index.js

@@ -9,6 +9,7 @@ const getPrefix = require('./lib/get-prefix.js')
 const lifecycle = require('npm-lifecycle')
 const lockVerify = require('lock-verify')
 const path = require('path')
+const logi = require('npm-logical-tree')
 const rimraf = BB.promisify(require('rimraf'))
 
 const readFileAsync = BB.promisify(fs.readFile)
@@ -19,22 +20,38 @@ class Installer {
 
     // Stats
     this.startTime = Date.now()
-    this.runTime = null
+    this.runTime = 0
     this.pkgCount = 0
 
     // Misc
     this.pkg = null
-    this.scriptQ = []
   }
 
   run () {
+    return this.prepare()
+    .then(() => this.runScript('preinstall', this.pkg, this.prefix))
+    .then(() => this.extractTree(this.logicalTree))
+    .then(() => this.runScript('install', this.pkg, this.prefix))
+    .then(() => this.runScript('postinstall', this.pkg, this.prefix))
+    .then(() => {
+      extract.stopWorkers()
+      this.runTime = Date.now() - this.startTime
+      return this
+    }, e => {
+      extract.stopWorkers()
+      throw e
+    })
+  }
+
+  prepare () {
     extract.startWorkers()
 
     return (
       this.opts.prefix
       ? BB.resolve(this.opts.prefix)
       : getPrefix(process.cwd())
-    ).then(prefix => {
+    )
+    .then(prefix => {
       this.prefix = prefix
       return BB.join(
         readJson(prefix, 'package.json'),
@@ -45,32 +62,17 @@ class Installer {
           this.pkg = pkg
         }
       )
-    }).then(() => {
-      return config(this.prefix, process.argv, this.pkg)
-    }).then(conf => {
+    })
+    .then(() => config(this.prefix, process.argv, this.pkg))
+    .then(conf => {
       this.config = conf
       return BB.join(
         this.checkLock(),
         rimraf(path.join(this.prefix, 'node_modules'))
       )
     }).then(() => {
-      return this.runScript('preinstall', this.pkg, this.prefix)
-    }).then(() => {
-      return this.extractDeps(
-        path.join(this.prefix, 'node_modules'),
-        this.pkg._shrinkwrap.dependencies
-      )
-    }).then(() => {
-      return this.runScript('install', this.pkg, this.prefix)
-    }).then(() => {
-      return this.runScript('postinstall', this.pkg, this.prefix)
-    }).then(() => {
-      extract.stopWorkers()
-      this.runTime = Date.now() - this.startTime
-      return this
-    }, e => {
-      extract.stopWorkers()
-      throw e
+      // This needs to happen -after- we've done checkLock()
+      this.logicalTree = logi(this.pkg, this.pkg._shrinkwrap)
     })
   }
 
@@ -97,36 +99,31 @@ class Installer {
     })
   }
 
-  extractDeps (modPath, deps) {
-    return BB.map(Object.keys(deps || {}), name => {
-      const child = deps[name]
-      const childPath = path.join(modPath, name)
-      return extract.child(name, child, childPath, this.config).then(() => {
-        return readJson(childPath, 'package.json')
-      }).tap(pkg => {
+  extractTree (tree) {
+    const deps = tree.dependencies.values()
+    return BB.map(deps, child => {
+      if (child.pending) { return hasCycle(child) || child.pending }
+      if (child.dev && this.config.config.production) { return }
+      const childPath = path.join(
+        this.prefix,
+        'node_modules',
+        child.address.replace(/:/g, '/node_modules/')
+      )
+      child.pending = BB.resolve()
+      .then(() => extract.child(child.name, child, childPath, this.config))
+      .then(() => readJson(childPath, 'package.json'))
+      .then(pkg => {
         return this.runScript('preinstall', pkg, childPath)
-      }).then(pkg => {
-        return this.extractDeps(
-          path.join(childPath, 'node_modules'), child.dependencies
-        ).then(dependencies => {
-          return {
-            name,
-            package: pkg,
-            child,
-            childPath,
-            dependencies: dependencies.reduce((acc, dep) => {
-              acc[dep.name] = dep
-              return acc
-            }, {})
-          }
+        .then(() => this.extractTree(child))
+        .then(() => this.runScript('install', pkg, childPath))
+        .then(() => this.runScript('postinstall', pkg, childPath))
+        .then(() => {
+          this.pkgCount++
+          return this
         })
-      }).tap(full => {
-        this.pkgCount++
-        return this.runScript('install', full.package, childPath)
-      }).tap(full => {
-        return this.runScript('postinstall', full.package, childPath)
       })
-    }, {concurrency: 50})
+      return child.pending
+    }, { concurrency: 50 })
   }
 
   runScript (stage, pkg, pkgPath) {
@@ -154,3 +151,14 @@ function readJson (jsonPath, name, ignoreMissing) {
 function stripBOM (str) {
   return str.replace(/^\uFEFF/, '')
 }
+
+function hasCycle (child, seen) {
+  seen = seen || new Set()
+  if (seen.has(child.address)) {
+    return true
+  } else {
+    seen.add(child.address)
+    const deps = Array.from(child.dependencies.values())
+    return deps.some(dep => hasCycle(dep, seen))
+  }
+}

package.json

@@ -37,6 +37,7 @@
     "graceful-fs": "^4.1.11",
     "lock-verify": "^1.1.0",
     "npm-lifecycle": "^1.0.3",
+    "npm-logical-tree": "^1.0.0",
     "npm-package-arg": "^5.1.2",
     "npmlog": "^4.1.2",
     "pacote": "^6.0.2",

test/specs/index.js

@@ -115,11 +115,16 @@ test('handles dependency list with only shallow subdeps', t => {
   const prefix = fixtureHelper.write(pkgName, {
     'package.json': {
       name: pkgName,
-      version: pkgVersion
+      version: pkgVersion,
+      dependencies: {
+        'a': '^1'
+      }
     },
     'package-lock.json': {
       dependencies: {
-        a: {}
+        a: {
+          version: '1.1.1'
+        }
       },
       lockfileVersion: 1
     }
@@ -150,13 +155,22 @@ test('handles dependency list with only deep subdeps', t => {
   const prefix = fixtureHelper.write(pkgName, {
     'package.json': {
       name: pkgName,
-      version: pkgVersion
+      version: pkgVersion,
+      dependencies: {
+        a: '^1'
+      }
     },
     'package-lock.json': {
       dependencies: {
         a: {
+          version: '1.1.1',
+          requires: {
+            b: '2.2.2'
+          },
           dependencies: {
-            b: {}
+            b: {
+              version: '2.2.2'
+            }
           }
         }
       },
@@ -206,11 +220,14 @@ test('runs lifecycle hooks of packages with env variables', t => {
         preinstall: writeEnvScript,
         install: writeEnvScript,
         postinstall: writeEnvScript
+      },
+      dependencies: {
+        a: '^1'
       }
     },
     'package-lock.json': {
       dependencies: {
-        a: {}
+        a: { version: '1.0.0' }
       },
       lockfileVersion: 1
     }
@@ -252,6 +269,7 @@ test('skips lifecycle scripts with ignoreScripts is set', t => {
     'package.json': {
       name: pkgName,
       version: pkgVersion,
+      dependencies: { a: '^1' },
       scripts: {
         preinstall: writeEnvScript,
         install: writeEnvScript,
@@ -260,7 +278,7 @@ test('skips lifecycle scripts with ignoreScripts is set', t => {
     },
     'package-lock.json': {
       dependencies: {
-        a: {}
+        a: { version: '1.0.0' }
       },
       lockfileVersion: 1
     }