lifecycle: cache config options and remove boolean traps

PR-URL: npm/npm#18025
Credit: @mikesherov
Reviewed-By: @zkat

lib/config/lifecycle.js

@@ -4,18 +4,25 @@ const npm = require('../npm.js')
 
 module.exports = lifecycleOpts
 
-function lifecycleOpts () {
-  return {
-    config: npm.config.snapshot,
-    dir: npm.dir,
-    force: npm.config.get('force'),
-    group: npm.config.get('group'),
-    ignorePrepublish: npm.config.get('ignore-prepublish'),
-    ignoreScripts: npm.config.get('ignore-scripts'),
-    production: npm.config.get('production'),
-    scriptShell: npm.config.get('script-shell'),
-    scriptsPrependNodePath: npm.config.get('scripts-prepend-node-path'),
-    unsafePerm: npm.config.get('unsafe-perm'),
-    user: npm.config.get('user')
+let opts
+
+function lifecycleOpts (moreOpts) {
+  if (!opts) {
+    opts = {
+      config: npm.config.snapshot,
+      dir: npm.dir,
+      failOk: false,
+      force: npm.config.get('force'),
+      group: npm.config.get('group'),
+      ignorePrepublish: npm.config.get('ignore-prepublish'),
+      ignoreScripts: npm.config.get('ignore-scripts'),
+      production: npm.config.get('production'),
+      scriptShell: npm.config.get('script-shell'),
+      scriptsPrependNodePath: npm.config.get('scripts-prepend-node-path'),
+      unsafePerm: npm.config.get('unsafe-perm'),
+      user: npm.config.get('user')
+    }
   }
+
+  return moreOpts ? Object.assign({}, opts, moreOpts) : opts
 }

lib/install/action/install.js

@@ -4,5 +4,5 @@ var packageId = require('../../utils/package-id.js')
 
 module.exports = function (staging, pkg, log, next) {
   log.silly('install', packageId(pkg))
-  lifecycle(pkg.package, 'install', pkg.path, false, false, next)
+  lifecycle(pkg.package, 'install', pkg.path, next)
 }

lib/install/action/move.js

@@ -19,12 +19,12 @@ var move = require('../../utils/move.js')
 module.exports = function (staging, pkg, log, next) {
   log.silly('move', pkg.fromPath, pkg.path)
   chain([
-    [lifecycle, pkg.package, 'preuninstall', pkg.fromPath, false, true],
-    [lifecycle, pkg.package, 'uninstall', pkg.fromPath, false, true],
+    [lifecycle, pkg.package, 'preuninstall', pkg.fromPath, { failOk: true }],
+    [lifecycle, pkg.package, 'uninstall', pkg.fromPath, { failOk: true }],
     [rmStuff, pkg.package, pkg.fromPath],
-    [lifecycle, pkg.package, 'postuninstall', pkg.fromPath, false, true],
+    [lifecycle, pkg.package, 'postuninstall', pkg.fromPath, { failOk: true }],
     [moveModuleOnly, pkg.fromPath, pkg.path, log],
-    [lifecycle, pkg.package, 'preinstall', pkg.path, false, true],
+    [lifecycle, pkg.package, 'preinstall', pkg.path, { failOk: true }],
     [removeEmptyParents, path.resolve(pkg.fromPath, '..')],
     [updatePackageJson, pkg, pkg.path]
   ], next)

lib/install/action/postinstall.js

@@ -4,5 +4,5 @@ var packageId = require('../../utils/package-id.js')
 
 module.exports = function (staging, pkg, log, next) {
   log.silly('postinstall', packageId(pkg))
-  lifecycle(pkg.package, 'postinstall', pkg.path, false, false, next)
+  lifecycle(pkg.package, 'postinstall', pkg.path, next)
 }

lib/install/action/preinstall.js

@@ -4,5 +4,5 @@ var packageId = require('../../utils/package-id.js')
 
 module.exports = function (staging, pkg, log, next) {
   log.silly('preinstall', packageId(pkg))
-  lifecycle(pkg.package, 'preinstall', pkg.path, false, false, next)
+  lifecycle(pkg.package, 'preinstall', pkg.path, next)
 }

lib/install/action/prepare.js

@@ -19,8 +19,8 @@ module.exports = function (staging, pkg, log, next) {
   var buildpath = moduleStagingPath(staging, pkg)
   chain(
     [
-      [lifecycle, pkg.package, 'prepublish', buildpath, false, false],
-      [lifecycle, pkg.package, 'prepare', buildpath, false, false]
+      [lifecycle, pkg.package, 'prepublish', buildpath],
+      [lifecycle, pkg.package, 'prepare', buildpath]
     ],
     next
   )

lib/install/action/unbuild.js

@@ -6,11 +6,11 @@ var rmStuff = Bluebird.promisify(require('../../unbuild.js').rmStuff)
 
 module.exports = function (staging, pkg, log) {
   log.silly('unbuild', packageId(pkg))
-  return lifecycle(pkg.package, 'preuninstall', pkg.path, false, true).then(() => {
-    return lifecycle(pkg.package, 'uninstall', pkg.path, false, true)
+  return lifecycle(pkg.package, 'preuninstall', pkg.path, { failOk: true }).then(() => {
+    return lifecycle(pkg.package, 'uninstall', pkg.path, { failOk: true })
   }).then(() => {
     return rmStuff(pkg.package, pkg.path)
   }).then(() => {
-    return lifecycle(pkg.package, 'postuninstall', pkg.path, false, true)
+    return lifecycle(pkg.package, 'postuninstall', pkg.path, { failOk: true })
   })
 }

lib/run-script.js

@@ -166,7 +166,7 @@ function run (pkg, wd, cmd, args, cb) {
     }
 
     // when running scripts explicitly, assume that they're trusted.
-    return [lifecycle, pkg, c, wd, true]
+    return [lifecycle, pkg, c, wd, { unsafePerm: true }]
   }), cb)
 }
 

lib/unbuild.js

@@ -38,14 +38,14 @@ function unbuild_ (silent) {
       if (er) return gentlyRm(folder, false, base, cb)
       chain(
         [
-          [lifecycle, pkg, 'preuninstall', folder, false, true],
-          [lifecycle, pkg, 'uninstall', folder, false, true],
+          [lifecycle, pkg, 'preuninstall', folder, { failOk: true }],
+          [lifecycle, pkg, 'uninstall', folder, { failOk: true }],
           !silent && function (cb) {
             output('unbuild ' + pkg._id)
             cb()
           },
           [rmStuff, pkg, folder],
-          [lifecycle, pkg, 'postuninstall', folder, false, true],
+          [lifecycle, pkg, 'postuninstall', folder, { failOk: true }],
           [gentlyRm, folder, false, base]
         ],
         cb

lib/utils/lifecycle.js

@@ -28,25 +28,17 @@ function logid (pkg, stage) {
   return pkg._id + '~' + stage + ':'
 }
 
-function runLifecycle (pkg, stage, wd, unsafe, failOk, cb) {
-  const opts = lifecycleOpts()
-  return lifecycle(pkg, stage, wd, unsafe, failOk, cb, opts)
-}
-
-function lifecycle (pkg, stage, wd, unsafe, failOk, cb, opts) {
-  if (typeof cb !== 'function') {
-    cb = failOk
-    failOk = false
-  }
-  if (typeof cb !== 'function') {
-    cb = unsafe
-    unsafe = false
-  }
-  if (typeof cb !== 'function') {
-    cb = wd
-    wd = null
+function runLifecycle (pkg, stage, wd, moreOpts, cb) {
+  if (typeof moreOpts === 'function') {
+    cb = moreOpts
+    moreOpts = null
   }
 
+  const opts = lifecycleOpts(moreOpts)
+  return lifecycle(pkg, stage, wd, opts, cb)
+}
+
+function lifecycle (pkg, stage, wd, opts, cb) {
   while (pkg && pkg._data) pkg = pkg._data
   if (!pkg) return cb(new Error('Invalid package data'))
 
@@ -67,10 +59,8 @@ function lifecycle (pkg, stage, wd, unsafe, failOk, cb, opts) {
   validWd(wd || path.resolve(opts.dir, pkg.name), function (er, wd) {
     if (er) return cb(er)
 
-    unsafe = unsafe || opts.unsafePerm
-
     if ((wd.indexOf(opts.dir) !== 0 || _incorrectWorkingDirectory(wd, pkg)) &&
-        !unsafe && pkg.scripts[stage]) {
+        !opts.unsafePerm && pkg.scripts[stage]) {
       log.warn('lifecycle', logid(pkg, stage), 'cannot run in wd',
         '%s %s (wd=%s)', pkg._id, pkg.scripts[stage], wd
       )
@@ -88,15 +78,15 @@ function lifecycle (pkg, stage, wd, unsafe, failOk, cb, opts) {
     // even if it's never used, sh freaks out.
     if (!opts.unsafePerm) env.TMPDIR = wd
 
-    lifecycle_(pkg, stage, wd, opts, env, unsafe, failOk, cb)
+    lifecycle_(pkg, stage, wd, opts, env, cb)
   })
 }
 
 function _incorrectWorkingDirectory (wd, pkg) {
   return wd.lastIndexOf(pkg.name) !== wd.length - pkg.name.length
 }
 
-function lifecycle_ (pkg, stage, wd, opts, env, unsafe, failOk, cb) {
+function lifecycle_ (pkg, stage, wd, opts, env, cb) {
   var pathArr = []
   var p = wd.split(/[\\\/]node_modules[\\\/]/)
   var acc = path.resolve(p.shift())
@@ -133,7 +123,7 @@ function lifecycle_ (pkg, stage, wd, opts, env, unsafe, failOk, cb) {
       if (opts.force) {
         log.info('lifecycle', logid(pkg, stage), 'forced, continuing', er)
         er = null
-      } else if (failOk) {
+      } else if (opts.failOk) {
         log.warn('lifecycle', logid(pkg, stage), 'continuing anyway', er.message)
         er = null
       }
@@ -143,8 +133,8 @@ function lifecycle_ (pkg, stage, wd, opts, env, unsafe, failOk, cb) {
 
   chain(
     [
-      packageLifecycle && [runPackageLifecycle, pkg, env, wd, opts, unsafe],
-      [runHookLifecycle, pkg, env, wd, opts, unsafe]
+      packageLifecycle && [runPackageLifecycle, pkg, env, wd, opts],
+      [runHookLifecycle, pkg, env, wd, opts]
     ],
     done
   )
@@ -196,14 +186,14 @@ function validWd (d, cb) {
   })
 }
 
-function runPackageLifecycle (pkg, env, wd, opts, unsafe, cb) {
+function runPackageLifecycle (pkg, env, wd, opts, cb) {
   // run package lifecycle scripts in the package root, or the nearest parent.
   var stage = env.npm_lifecycle_event
   var cmd = env.npm_lifecycle_script
 
   var note = '\n> ' + pkg._id + ' ' + stage + ' ' + wd +
              '\n> ' + cmd + '\n'
-  runCmd(note, cmd, pkg, env, stage, wd, opts, unsafe, cb)
+  runCmd(note, cmd, pkg, env, stage, wd, opts, cb)
 }
 
 var running = false
@@ -216,14 +206,15 @@ function dequeue () {
   }
 }
 
-function runCmd (note, cmd, pkg, env, stage, wd, opts, unsafe, cb) {
+function runCmd (note, cmd, pkg, env, stage, wd, opts, cb) {
   if (running) {
-    queue.push([note, cmd, pkg, env, stage, wd, unsafe, cb])
+    queue.push([note, cmd, pkg, env, stage, wd, cb])
     return
   }
 
   running = true
   log.pause()
+  var unsafe = opts.unsafePerm
   var user = unsafe ? null : opts.user
   var group = unsafe ? null : opts.group
 
@@ -333,7 +324,7 @@ function runCmd_ (cmd, pkg, env, wd, opts, stage, unsafe, uid, gid, cb_) {
   }
 }
 
-function runHookLifecycle (pkg, env, wd, opts, unsafe, cb) {
+function runHookLifecycle (pkg, env, wd, opts, cb) {
   // check for a hook script, run if present.
   var stage = env.npm_lifecycle_event
   var hook = path.join(opts.dir, '.hooks', stage)
@@ -343,7 +334,7 @@ function runHookLifecycle (pkg, env, wd, opts, unsafe, cb) {
     if (er) return cb()
     var note = '\n> ' + pkg._id + ' ' + stage + ' ' + wd +
                '\n> ' + cmd
-    runCmd(note, hook, pkg, env, stage, wd, opts, unsafe, cb)
+    runCmd(note, hook, pkg, env, stage, wd, opts, cb)
   })
 }
 

test/tap/verify-no-lifecycle-on-repo.js

@@ -23,7 +23,11 @@ var baseJSON = {
 
 var lastOpened
 var npm = requireInject.installGlobally('../../lib/npm.js', {
-  '../../lib/utils/lifecycle.js': function (pkg, stage, wd, unsafe, failOk, cb) {
+  '../../lib/utils/lifecycle.js': function (pkg, stage, wd, moreOpts, cb) {
+    if (typeof moreOpts === 'function') {
+      cb = moreOpts
+    }
+
     cb(new Error("Shouldn't be calling lifecycle scripts"))
   },
   opener: function (url, options, cb) {