fixup! fix: hide banner for exec and explore

Author: lukekarrys

lib/npm.js

@@ -32,6 +32,7 @@ class Npm {
   updateNotification = null
   argv = []
 
+  #commandName = null
   #command = null
   #runId = new Date().toISOString().replace(/[.:]/g, '_')
   #title = 'npm'
@@ -94,11 +95,11 @@ class Npm {
 
   async load () {
     return time.start('npm:load', async () => {
-      const { exec } = await this.#load()
+      const { exec, command, args } = await this.#load()
       return {
         exec,
-        command: this.argv.shift(),
-        args: this.argv,
+        command,
+        args,
       }
     })
   }
@@ -165,7 +166,26 @@ class Npm {
 
     await time.start('npm:load:configload', () => this.config.load())
 
+    // npm --versions
+    if (this.config.get('versions', 'cli')) {
+      this.argv = ['version']
+      this.config.set('usage', false, 'cli')
+    } else {
+      this.argv = [...this.config.parsedArgv.remain]
+    }
+
+    // Remove first argv since that is our command as typed
+    // Note that this might not be the actual name of the command
+    // due to aliases, etc. But we use the raw form of it later
+    // in user output so it must be preserved as is.
+    const commandArg = this.argv.shift()
+
+    // This is the actual name of the command that will be run or
+    // undefined if deref could not find a match
+    const command = deref(commandArg)
+
     await this.#display.load({
+      command
       loglevel: this.config.get('loglevel'),
       stdoutColor: this.color,
       stderrColor: this.logColor,
@@ -174,9 +194,6 @@ class Npm {
       progress: this.flatOptions.progress,
       json: this.config.get('json'),
       heading: this.config.get('heading'),
-      // TODO: avoid passing all of npm in. this is a quick
-      // fix to allow display to know the currently running command
-      npm: this,
     })
     process.env.COLOR = this.color ? '1' : '0'
 
@@ -205,23 +222,24 @@ class Npm {
 
     // note: this MUST be shorter than the actual argv length, because it
     // uses the same memory, so node will truncate it if it's too long.
+    // We time this because setting process.title is slow sometimes but we
+    // have to do it for security reasons. But still helpful to know how slow it is.
     time.start('npm:load:setTitle', () => {
-      const { parsedArgv: { cooked, remain } } = this.config
-      this.argv = remain
       // Secrets are mostly in configs, so title is set using only the positional args
       // to keep those from being leaked.  We still do a best effort replaceInfo.
-      this.#title = ['npm'].concat(replaceInfo(remain)).join(' ').trim()
+      this.#title = ['npm'].concat(replaceInfo(this.config.parsedArgv.remain)).join(' ').trim()
       process.title = this.#title
-      // The cooked argv is also logged separately for debugging purposes. It is
-      // cleaned as a best effort by replacing known secrets like basic auth
-      // password and strings that look like npm tokens. XXX: for this to be
-      // safer the config should create a sanitized version of the argv as it
-      // has the full context of what each option contains.
-      this.#argvClean = replaceInfo(cooked)
-      log.verbose('title', this.title)
-      log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' '))
     })
 
+    // The cooked argv is also logged separately for debugging purposes. It is
+    // cleaned as a best effort by replacing known secrets like basic auth
+    // password and strings that look like npm tokens. XXX: for this to be
+    // safer the config should create a sanitized version of the argv as it
+    // has the full context of what each option contains.
+    this.#argvClean = replaceInfo(this.config.parsedArgv.cooked)
+    log.verbose('title', this.title)
+    log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' '))
+
     // logFile.load returns a promise that resolves when old logs are done being cleaned.
     // We save this promise to an array so that we can await it in tests to ensure more
     // deterministic logging behavior. The process will also hang open if this were to
@@ -247,13 +265,7 @@ class Npm {
       log.warn('using --force', 'Recommended protections disabled.')
     }
 
-    // npm --versions
-    if (this.config.get('versions', 'cli')) {
-      this.argv = ['version']
-      this.config.set('usage', false, 'cli')
-    }
-
-    return { exec: true }
+    return { exec: true, command: commandArg, args: this.argv }
   }
 
   get isShellout () {

lib/utils/display.js

@@ -119,12 +119,12 @@ class Display {
   #progress
 
   // options
+  #command
   #levelIndex
   #timing
   #json
   #heading
   #silent
-  #npm
 
   // display streams
   #stdout
@@ -160,6 +160,7 @@ class Display {
   }
 
   async load ({
+    command,
     heading,
     json,
     loglevel,
@@ -168,9 +169,8 @@ class Display {
     stdoutColor,
     timing,
     unicode,
-    npm,
   }) {
-    this.#npm = npm
+    this.#command = command
     // get createSupportsColor from chalk directly if this lands
     // https://github.com/chalk/chalk/pull/600
     const [{ Chalk }, { createSupportsColor }] = await Promise.all([
@@ -275,10 +275,13 @@ class Display {
       return
     }
 
-    // HACK: if it looks like the banner and we are silent do not print it.
-    // There's no other way to do this right now :(\
-    const isBanner = args.length === 1 && args[0].startsWith('\n> ') && args[0].endsWith('\n')
-    const hideBanner = this.#silent || ['exec', 'explore'].includes(this.#npm.command)
+    // HACK: if it looks like the banner and we are in a state where we hide the
+    // banner then dont write any output. This hack can be replaced with proc-log.META
+    const isBanner = args.length === 1 &&
+      typeof args[0] === 'string' &&
+      args[0].startsWith('\n> ') &&
+      args[0].endsWith('\n')
+    const hideBanner = this.#silent || ['exec', 'explore'].includes(this.#command)
     if (isBanner && hideBanner) {
       return
     }

test/lib/docs.js

@@ -74,7 +74,7 @@ t.test('basic usage', async t => {
   // are generated in the following test
   const { npm } = await loadMockNpm(t, {
     mocks: {
-      '{LIB}/utils/cmd-list.js': { commands: [] },
+      '{LIB}/utils/cmd-list.js': { ...cmdList, commands: [] },
     },
     config: { userconfig: '/some/config/file/.npmrc' },
     globals: { process: { platform: 'posix' } },