fix: unwrap comments and lines meant for output (#9087)

Author: wraithgar

lib/arborist-cmd.js

@@ -1,9 +1,7 @@
 const { log } = require('proc-log')
 const BaseCommand = require('./base-cmd.js')
 
-// This is the base for all commands whose execWorkspaces just gets
-// a list of workspace names and passes it on to new Arborist() to
-// be able to run a filtered Arborist.reify() at some point.
+// This is the base for all commands whose execWorkspaces just gets a list of workspace names and passes it on to new Arborist() to be able to run a filtered Arborist.reify() at some point.
 class ArboristCmd extends BaseCommand {
   get isArboristCmd () {
     return true
@@ -25,8 +23,7 @@ class ArboristCmd extends BaseCommand {
 
     const { config } = this.npm
 
-    // when location isn't set and global isn't true check for a package.json at
-    // the localPrefix and set the location to project if found
+    // when location isn't set and global isn't true check for a package.json at the localPrefix and set the location to project if found
     const locationProject = config.get('location') === 'project' || (
       config.isDefault('location')
       // this is different then `npm.global` which falls back to checking
@@ -35,8 +32,7 @@ class ArboristCmd extends BaseCommand {
       && npm.localPackage
     )
 
-    // if audit is not set and we are in global mode and location is not project
-    // and we assume its not a project related context, then we set audit=false
+    // if audit is not set and we are in global mode and location is not project and we assume its not a project related context, then we set audit=false
     if (config.isDefault('audit') && (this.npm.global || !locationProject)) {
       config.set('audit', false)
     } else if (this.npm.global && config.get('audit')) {

lib/base-cmd.js

@@ -17,8 +17,7 @@ class BaseCommand {
   // Number of expected positional arguments (null = unlimited/unchecked)
   static positionals = null
 
-  // this is a static so that we can read from it without instantiating a command
-  // which would require loading the config
+  // this is a static so that we can read from it without instantiating a command which would require loading the config
   static get describeUsage () {
     return this.getUsage()
   }
@@ -29,8 +28,7 @@ class BaseCommand {
     const wrapWidth = 80
     const { description, usage = [''], name } = this
 
-    // Resolve to a definitions array: if the command has its own definitions, use
-    // those directly; otherwise resolve params from the global definitions pool.
+    // Resolve to a definitions array: if the command has its own definitions, use those directly; otherwise resolve params from the global definitions pool.
     let cmdDefs
     if (this.definitions) {
       cmdDefs = this.definitions
@@ -392,8 +390,7 @@ class BaseCommand {
       throw this.usageError(`Unknown flag${unknownFlags.length > 1 ? 's' : ''}: ${flagList}`)
     }
 
-    // Remove warnings for command-specific definitions that npm's global config
-    // doesn't know about (these were queued as "unknown" during config.load())
+    // Remove warnings for command-specific definitions that npm's global config doesn't know about (these were queued as "unknown" during config.load())
     for (const def of commandDefinitions) {
       this.npm.config.removeWarning(def.key)
       if (def.alias && Array.isArray(def.alias)) {
@@ -403,8 +400,7 @@ class BaseCommand {
       }
     }
 
-    // Remove warnings for unknown positionals that were actually consumed as flag values
-    // by command-specific definitions (e.g., --id <value> where --id is command-specific)
+    // Remove warnings for unknown positionals that were actually consumed as flag values by command-specific definitions (e.g., --id <value> where --id is command-specific)
     const remainsSet = new Set(remains)
     for (const unknownPos of this.npm.config.getUnknownPositionals()) {
       if (!remainsSet.has(unknownPos)) {

lib/cli/entry.js

@@ -1,7 +1,6 @@
 // Separated out for easier unit testing
 module.exports = async (process, validateEngines) => {
-  // set it here so that regardless of what happens later, we don't
-  // leak any private CLI configs to other programs
+  // set it here so that regardless of what happens later, we don't leak any private CLI configs to other programs
   process.title = 'npm'
 
   // Patch the global fs module here at the app level
@@ -20,11 +19,13 @@ module.exports = async (process, validateEngines) => {
   log.info('using', 'npm@%s', npm.version)
   log.info('using', 'node@%s', process.version)
 
-  // At this point we've required a few files and can be pretty sure we don't contain invalid syntax for this version of node. It's possible a lazy require would, but that's unlikely enough that it's not worth catching anymore and we attach the more important exit handlers.
+  // At this point we've required a few files and can be pretty sure we don't contain invalid syntax for this version of node.
+  // It's possible a lazy require would, but that's unlikely enough that it's not worth catching anymore and we attach the more important exit handlers.
   validateEngines.off()
   exitHandler.registerUncaughtHandlers()
 
-  // It is now safe to log a warning if they are using a version of node that is not going to fail on syntax errors but is still unsupported and untested and might not work reliably. This is safe to use the logger now which we want since this will show up in the error log too.
+  // It is now safe to log a warning if they are using a version of node that is not going to fail on syntax errors but is still unsupported and untested and might not work reliably.
+  // This is safe to use the logger now which we want since this will show up in the error log too.
   if (!satisfies(validateEngines.node, validateEngines.engines)) {
     log.warn('cli', validateEngines.unsupportedMessage)
   }
@@ -57,9 +58,8 @@ module.exports = async (process, validateEngines) => {
 
     const execPromise = npm.exec(command, args)
 
-    // this is async but we don't await it, since its ok if it doesnt
-    // finish before the command finishes running. it uses command and argv
-    // so it must be initiated here, after the command name is set
+    // this is async but we don't await it, since its ok if it doesnt finish before the command finishes running.
+    // it uses command and argv so it must be initiated here, after the command name is set
     const updateNotifier = require('./update-notifier.js')
     // eslint-disable-next-line promise/catch-or-return
     updateNotifier(npm).then((msg) => (npm.updateNotification = msg))

lib/cli/exit-handler.js

@@ -52,8 +52,7 @@ class ExitHandler {
   #handleProcessExitAndReset = (code) => {
     this.#handleProcessExit(code)
 
-    // Reset all the state. This is only relevant for tests since
-    // in reality the process fully exits here.
+    // Reset all the state. This is only relevant for tests since in reality the process fully exits here.
     this.#process.off('exit', this.#handleProcessExitAndReset)
     this.#process.off('uncaughtException', this.#handleExit)
     this.#process.off('unhandledRejection', this.#handleExit)
@@ -115,9 +114,8 @@ class ExitHandler {
   }
 
   #logConsoleError (err) {
-    // Run our error message formatters on all errors even if we
-    // have no npm or an unloaded npm. This will clean the error
-    // and possible return a formatted message about EACCESS or something.
+    // Run our error message formatters on all errors even if we have no npm or an unloaded npm.
+    // This will clean the error and possible return a formatted message about EACCESS or something.
     const { summary, detail } = errorMessage(err, this.#npm)
     const formatted = [...new Set([...summary, ...detail].flat().filter(Boolean))].join('\n')
     // If we didn't get anything from the formatted message then just display the full stack
@@ -147,24 +145,20 @@ class ExitHandler {
       return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
     }
 
-    // npm was never loaded but we still might have a config loading error or
-    // something similar that we can run through the error message formatter
-    // to give the user a clue as to what happened.s
+    // npm was never loaded but we still might have a config loading error or something similar that we can run through the error message formatter to give the user a clue as to what happened.
     if (!this.#loaded) {
       this.#logConsoleError(new Error('Exit prior to config file resolving', { cause: err }))
       return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
     }
 
     this.#exitErrorMessage = err?.suppressError === true ? false : !!err
 
-    // Prefer the exit code of the error, then the current process exit code,
-    // then set it to 1 if we still have an error. Otherwise, we call process.exit
-    // with undefined so that it can determine the final exit code
+    // Prefer the exit code of the error, then the current process exit code, then set it to 1 if we still have an error.
+    // Otherwise, we call process.exit with undefined so that it can determine the final exit code
     const exitCode = err?.exitCode ?? this.#process.exitCode ?? (err ? 1 : undefined)
 
-    // explicitly call process.exit now so we don't hang on things like the
-    // update notifier, also flush stdout/err beforehand because process.exit doesn't
-    // wait for that to happen.
+    // explicitly call process.exit now so we don't hang on things like the update notifier
+    // also flush stdout/err beforehand because process.exit doesn't wait for that to happen.
     this.#process.stderr.write('', () => this.#process.stdout.write('', () => {
       this.#process.exit(exitCode)
     }))

lib/cli/update-notifier.js

@@ -1,6 +1,4 @@
-// print a banner telling the user to upgrade npm to latest
-// but not in CI, and not if we're doing that already.
-// Check daily for betas, and weekly otherwise.
+// print a banner telling the user to upgrade npm to latest but not in CI, and not if we're doing that already.
 
 const ciInfo = require('ci-info')
 const gt = require('semver/functions/gt')
@@ -14,11 +12,9 @@ const DAILY = 1000 * 60 * 60 * 24
 const WEEKLY = DAILY * 7
 
 // don't put it in the _cacache folder, just in npm's cache
-const lastCheckedFile = npm =>
-  resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
+const lastCheckedFile = npm => resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
 
-// Actual check for updates. This is a separate function so that we only load
-// this if we are doing the actual update
+// Actual check for updates. This is a separate function so that we only load this if we are doing the actual update
 const updateCheck = async (npm, spec, version, current) => {
   const pacote = require('pacote')
 
@@ -36,10 +32,8 @@ const updateCheck = async (npm, spec, version, current) => {
 
   const latest = mani.version
 
-  // if the current version is *greater* than latest, we're on a 'next'
-  // and should get the updates from that release train.
-  // Note that this isn't another http request over the network, because
-  // the packument will be cached by pacote from previous request.
+  // if the current version is *greater* than latest, we're on a 'next' and should get the updates from that release train.
+  // Note that this isn't another http request over the network, because the packument will be cached by pacote from previous request.
   if (gt(version, latest) && spec === '*') {
     return updateNotifier(npm, `^${version}`)
   }
@@ -51,9 +45,8 @@ const updateCheck = async (npm, spec, version, current) => {
 
   const chalk = npm.logChalk
 
-  // ok!  notify the user about this update they should get.
-  // The message is saved for printing at process exit so it will not get
-  // lost in any other messages being printed as part of the command.
+  // ok! notify the user about this update they should get.
+  // The message is saved for printing at process exit so it will not get lost in any other messages being printed as part of the command.
   const update = parse(mani.version)
   const type = update.major !== current.major ? 'major'
     : update.minor !== current.minor ? 'minor'
@@ -62,18 +55,20 @@ const updateCheck = async (npm, spec, version, current) => {
   const typec = type === 'major' ? 'red'
     : type === 'minor' ? 'yellow'
     : 'cyan'
-  const cmd = `npm install -g npm@${latest}`
-  const message = `\nNew ${chalk[typec](type)} version of npm available! ` +
-    `${chalk[typec](current)} -> ${chalk.blue(latest)}\n` +
-    `Changelog: ${chalk.blue(`https://github.com/npm/cli/releases/tag/v${latest}`)}\n` +
-    `To update run: ${chalk.underline(cmd)}\n`
+  const message = [
+    '',
+    `New ${chalk[typec](type)} version of npm available! ${chalk[typec](current)} -> ${chalk.blue(latest)}`,
+    `Changelog: ${chalk.blue(`https://github.com/npm/cli/releases/tag/v${latest}`)}`,
+    `To update run: ${chalk.underline(`npm install -g npm@${latest}`)}`,
+    '',
+  ].join('\n')
 
   return message
 }
 
 const updateNotifier = async (npm, spec = '*') => {
-  // if we're on a prerelease train, then updates are coming fast
-  // check for a new one daily.  otherwise, weekly.
+  // if we're on a prerelease train, then updates are coming fast check for a new one daily.
+  // otherwise, weekly.
   const { version } = npm
   const current = parse(version)
 
@@ -94,15 +89,15 @@ const updateNotifier = async (npm, spec = '*') => {
     return null
   }
 
-  // intentional.  do not await this.  it's a best-effort update.  if this
-  // fails, it's ok.  might be using /dev/null as the cache or something weird
-  // like that.
+  // intentional.  do not await this.  it's a best-effort update.
+  // if this fails, it's ok.
+  // might be using /dev/null as the cache or something weird like that.
   writeFile(lastCheckedFile(npm), '').catch(() => {})
 
   return updateCheck(npm, spec, version, current)
 }
 
-module.exports = npm => {
+module.exports = async npm => {
   if (
     // opted out
     !npm.config.get('update-notifier')
@@ -113,7 +108,7 @@ module.exports = npm => {
     // CI
     || ciInfo.isCI
   ) {
-    return Promise.resolve(null)
+    return null
   }
 
   return updateNotifier(npm)

lib/cli/validate-engines.js

@@ -1,9 +1,6 @@
-// This is separate to indicate that it should contain code we expect to work in
-// all versions of node >= 6.  This is a best effort to catch syntax errors to
-// give users a good error message if they are using a node version that doesn't
-// allow syntax we are using such as private properties, etc. This file is
-// linted with ecmaVersion=6 so we don't use invalid syntax, which is set in the
-// .eslintrc.local.json file
+// This is separate to indicate that it should contain code we expect to work in all versions of node >= 6.
+// This is a best effort to catch syntax errors to give users a good error message if they are using a node version that doesn't allow syntax we are using such as private properties, etc.
+// This file is linted with ecmaVersion=6 so we don't use invalid syntax, which is set in the .eslintrc.local.json file
 
 const { engines: { node: engines }, version } = require('../../package.json')
 const npm = `v${version}`
@@ -15,8 +12,7 @@ module.exports = (process, getCli) => {
 
   const brokenMessage = `ERROR: npm ${npm} is known not to run on Node.js ${node}.  This version of npm supports the following node versions: \`${engines}\`. You can find the latest version at https://nodejs.org/.`
 
-  // coverage ignored because this is only hit in very unsupported node versions
-  // and it's a best effort attempt to show something nice in those cases
+  // coverage ignored because this is only hit in very unsupported node versions and it's a best effort attempt to show something nice in those cases
   /* istanbul ignore next */
   const syntaxErrorHandler = (err) => {
     if (err instanceof SyntaxError) {

lib/commands/audit.js

@@ -36,7 +36,7 @@ class Audit extends ArboristWorkspaceCmd {
       case 'signatures':
         return []
       default:
-        throw Object.assign(new Error(argv[2] + ' not recognized'), {
+        throw Object.assign(new Error(`${argv[2]} not recognized`), {
           code: 'EUSAGE',
         })
     }

lib/commands/cache.js

@@ -131,20 +131,14 @@ class Cache extends BaseCommand {
     const cachePath = this.npm.flatOptions.cache
     if (args.length === 0) {
       if (!this.npm.config.get('force')) {
-        throw new Error(`As of npm@5, the npm cache self-heals from corruption issues
-  by treating integrity mismatches as cache misses.  As a result,
-  data extracted from the cache is guaranteed to be valid.  If you
-  want to make sure everything is consistent, use \`npm cache verify\`
-  instead.  Deleting the cache can only make npm go slower, and is
-  not likely to correct any problems you may be encountering!
+        throw new Error(`As of npm@5, the npm cache self-heals from corruption issues by treating integrity mismatches as cache misses.
+As a result, data extracted from the cache is guaranteed to be valid.
+If you want to make sure everything is consistent, use \`npm cache verify\` instead.
+Deleting the cache can only make npm go slower, and is not likely to correct any problems you may be encountering!
 
-  On the other hand, if you're debugging an issue with the installer,
-  or race conditions that depend on the timing of writing to an empty
-  cache, you can use \`npm install --cache /tmp/empty-cache\` to use a
-  temporary cache instead of nuking the actual one.
+On the other hand, if you're debugging an issue with the installer, or race conditions that depend on the timing of writing to an empty cache, you can use \`npm install --cache /tmp/empty-cache\` to use a temporary cache instead of removing the actual one.
 
-  If you're sure you want to delete the entire cache, rerun this command
-  with --force.`)
+If you're sure you want to delete the entire cache, rerun this command with --force.`)
       }
       return fs.rm(cachePath, { recursive: true, force: true })
     }
@@ -175,9 +169,7 @@ class Cache extends BaseCommand {
 
     await Promise.all(args.map(async spec => {
       log.silly('cache add', 'spec', spec)
-      // we ask pacote for the thing, and then just throw the data
-      // away so that it tee-pipes it into the cache like it does
-      // for a normal request.
+      // we ask pacote for the thing, and then just throw the data away so that it tee-pipes it into the cache like it does for a normal request.
       await pacote.tarball.stream(spec, stream => {
         stream.resume()
         return stream.promise()

lib/commands/ci.js

@@ -83,8 +83,7 @@ class CI extends ArboristWorkspaceCmd {
         includeWorkspaceRoot: true,
       })
 
-      // Only remove node_modules after we've successfully loaded the virtual
-      // tree and validated the lockfile
+      // Only remove node_modules after we've successfully loaded the virtual tree and validated the lockfile
       await time.start('npm-ci:rm', async () => {
         return await Promise.all([...workspacePaths.values()].map(async modulePath => {
           const fullPath = path.join(modulePath, 'node_modules')