fixup! feat: do all ouput over proc-log events

Author: lukekarrys

lib/npm.js

@@ -244,6 +244,9 @@ class Npm {
     this.time('npm:load:display', () => {
       this.#display.load({
         loglevel: this.config.get('loglevel'),
+        // TODO: only pass in logColor and color and create chalk instances
+        // in display load method. Then remove chalk getters from npm and
+        // producers should emit chalk-templates (or something else).
         stdoutChalk: this.#chalk,
         stdoutColor: this.color,
         stderrChalk: this.#logChalk,
@@ -436,10 +439,12 @@ class Npm {
     return usage(this)
   }
 
+  // TODO: move to proc-log and remove
   forceLog (...args) {
     this.#display.forceLog(...args)
   }
 
+  // TODO: move to proc-log and remove
   flushOutput (jsonError) {
     this.#display.flushOutput(jsonError)
   }

lib/utils/display.js

@@ -1,5 +1,5 @@
 const proggy = require('proggy')
-const { log } = require('proc-log')
+const { log, output } = require('proc-log')
 const { explain } = require('./explain-eresolve.js')
 const { formatWithOptions } = require('./format')
 
@@ -17,7 +17,13 @@ const COLOR_PALETTE = ({ chalk: c }) => ({
   silly: c.inverse,
 })
 
-const LEVELS = log.LEVELS.reduce((acc, key) => {
+const LOG_LEVELS = log.LEVELS.reduce((acc, key) => {
+  acc[key] = key
+  return acc
+}, {})
+
+// TODO: move flush to proc-log
+const OUTPUT_LEVELS = ['flush', ...output.LEVELS].reduce((acc, key) => {
   acc[key] = key
   return acc
 }, {})
@@ -55,20 +61,20 @@ const LEVEL_OPTIONS = {
 
 const LEVEL_METHODS = {
   ...LEVEL_OPTIONS,
-  [LEVELS.timing]: {
+  [LOG_LEVELS.timing]: {
     show: ({ timing, index }) => !!timing && index !== 0,
   },
 }
 
-const safeJsonParse = (maybeJsonStr) => {
-  if (typeof maybeJsonStr !== 'string') {
-    return maybeJsonStr
-  }
-  try {
-    return JSON.parse(maybeJsonStr)
-  } catch {
-    return {}
+const tryJsonParse = (value) => {
+  if (typeof value === 'string') {
+    try {
+      return JSON.parse(value)
+    } catch {
+      return {}
+    }
   }
+  return value
 }
 
 const setBlocking = (stream) => {
@@ -122,6 +128,7 @@ class Display {
   constructor ({ stdout, stderr }) {
     this.#stdout = setBlocking(stdout)
     this.#stderr = setBlocking(stderr)
+
     // Handlers are set immediately so they can buffer all events
     process.on('log', this.#logHandler)
     process.on('output', this.#outputHandler)
@@ -174,21 +181,34 @@ class Display {
     log.resume()
 
     // TODO: this should be a proc-log method `proc-log.output.flush`?
-    this.#outputHandler('flush')
+    this.#outputHandler(OUTPUT_LEVELS.flush)
 
     this.#startProgress({ progress, unicode })
   }
 
+  // STREAM WRITES
+
+  // Write formatted and (non-)colorized output to streams
+  #stdoutWrite (options, ...args) {
+    this.#stdout.write(formatWithOptions({ colors: this.#stdoutColor, ...options }, ...args))
+  }
+
+  #stderrWrite (options, ...args) {
+    this.#stderr.write(formatWithOptions({ colors: this.#stderrColor, ...options }, ...args))
+  }
+
+  // HANDLERS
+
   // Public class field so it can be passed directly as a listener
   #logHandler = (...args) => {
-    if (args[0] === LEVELS.resume) {
+    if (args[0] === LOG_LEVELS.resume) {
       this.#logState.buffering = false
       this.#logState.buffer.forEach((item) => this.#tryWriteLog(...item))
       this.#logState.buffer.length = 0
       return
     }
 
-    if (args[0] === LEVELS.pause) {
+    if (args[0] === LOG_LEVELS.pause) {
       this.#logState.buffering = true
       return
     }
@@ -203,23 +223,22 @@ class Display {
 
   // Public class field so it can be passed directly as a listener
   #outputHandler = (...args) => {
-    if (args[0] === 'flush') {
+    if (args[0] === OUTPUT_LEVELS.flush) {
       this.#outputState.buffering = false
       if (args[1] && this.#json) {
-        const mergedJsonOutput = {}
-        for (const [, ...items] of this.#outputState.buffer) {
-          Object.assign(mergedJsonOutput, ...items.map(safeJsonParse))
+        const json = {}
+        for (const [, item] of this.#outputState.buffer) {
+          Object.assign(json, tryJsonParse(item))
         }
-        Object.assign(args[1])
-        this.#writeOutput('standard', JSON.stringify(mergedJsonOutput, null, 2))
+        this.#writeOutput('standard', JSON.stringify({ ...json, ...args[1] }, null, 2))
       } else {
         this.#outputState.buffer.forEach((item) => this.#writeOutput(...item))
       }
       this.#outputState.buffer.length = 0
       return
     }
 
-    if (args[0] === 'buffer') {
+    if (args[0] === OUTPUT_LEVELS.buffer) {
       this.#outputState.buffer.push(['standard', ...args.slice(1)])
       return
     }
@@ -237,25 +256,27 @@ class Display {
   #writeOutput (...args) {
     const { level } = getLevel(args.shift())
 
-    if (level === 'standard') {
-      this.#stdout.write(formatWithOptions({ colors: this.#stdoutColor }, ...args))
+    if (level === OUTPUT_LEVELS.standard) {
+      this.#stdoutWrite({}, ...args)
       return
     }
 
-    if (level === 'error') {
-      this.#stderr.write(formatWithOptions({ colors: this.#stderrColor }, ...args))
+    if (level === OUTPUT_LEVELS.error) {
+      this.#stderrWrite({}, ...args)
     }
   }
 
   // TODO: move this to proc-log and remove this public method
   flushOutput (jsonError) {
-    this.#outputHandler('flush', jsonError)
+    this.#outputHandler(OUTPUT_LEVELS.flush, jsonError)
   }
 
   // LOGS
 
+  // TODO: make proc-log able to send signal data like `force`
+  // when that happens, remove this public method
   forceLog (level, ...args) {
-    // This will show the log regardless of the current loglevel
+    // This will show the log regardless of the current loglevel except when silent
     if (this.#levelIndex !== 0) {
       this.#logHandler({ level, force: true }, ...args)
     }
@@ -269,7 +290,7 @@ class Display {
       // highly abbreviated explanation of what's being overridden.
       // TODO: this could probably be moved to arborist now that display is refactored
       const [level, heading, message, expl] = args
-      if (level === LEVELS.warn && heading === 'ERESOLVE' && expl && typeof expl === 'object') {
+      if (level === LOG_LEVELS.warn && heading === 'ERESOLVE' && expl && typeof expl === 'object') {
         this.#writeLog(level, heading, message)
         this.#writeLog(level, '', explain(expl, this.#stderrChalk, 2))
         return
@@ -278,9 +299,10 @@ class Display {
     } catch (ex) {
       try {
         // if it crashed once, it might again!
-        this.#writeLog(LEVELS.verbose, null, `attempt to log crashed`, ...args, ex)
+        this.#writeLog(LOG_LEVELS.verbose, null, `attempt to log crashed`, ...args, ex)
       } catch (ex2) {
-        /* istanbul ignore next - this happens if the object has an inspect method that crashes */
+        // This happens if the object has an inspect method that crashes so just console.error
+        // with the errors but don't do anything else that might error again.
         // eslint-disable-next-line no-console
         console.error(`attempt to log crashed`, ex, ex2)
       }
@@ -301,7 +323,7 @@ class Display {
         this.#logColors[level](levelOpts.label ?? level),
         title ? this.#logColors.title(title) : null,
       ]
-      this.#stderr.write(formatWithOptions({ prefix, colors: this.stderrColor }, ...args))
+      this.#stderrWrite({ prefix }, ...args)
     } else if (this.#progress) {
       // TODO: make this display a single log line of filtered messages
     }

lib/utils/exit-handler.js

@@ -8,15 +8,6 @@ let npm = null // set by the cli
 let exitHandlerCalled = false
 let showLogFileError = false
 
-const outputError = (v) => {
-  if (npm) {
-    output.error(v)
-  } else {
-    // Use console for output if there is no npm
-    // eslint-disable-next-line no-console
-    console.error(v)
-  }
-}
 process.on('exit', code => {
   // process.emit is synchronous, so the timeEnd handler will run before the
   // unfinished timer check below
@@ -40,9 +31,13 @@ process.on('exit', code => {
   if (!exitHandlerCalled) {
     process.exitCode = code || 1
     log.error('', 'Exit handler never called!')
-    outputError('')
     log.error('', 'This is an error with npm itself. Please report this error at:')
     log.error('', '    <https://github.com/npm/cli/issues>')
+
+    // This gets logged regardless of silent mode
+    // eslint-disable-next-line no-console
+    console.error('')
+
     showLogFileError = true
   }
 
@@ -67,10 +62,7 @@ process.on('exit', code => {
     const logMethod = showLogFileError ? 'error' : timing ? 'info' : null
 
     if (logMethod) {
-      if (!npm.silent) {
-        // just a line break if not in silent mode
-        outputError('')
-      }
+      output.error('')
 
       const message = []
 
@@ -116,17 +108,20 @@ const exitHandler = err => {
     err = err || new Error('Exit prior to setting npm in exit handler')
     // Don't use proc-log here since npm was never set
     // eslint-disable-next-line no-console
-    outputError(err.stack || err.message)
+    console.error(err.stack || err.message)
     return process.exit(1)
   }
 
   if (!hasLoadedNpm) {
     err = err || new Error('Exit prior to config file resolving.')
-    outputError(err.stack || err.message)
+    // Don't use proc-log here since npm was never loaded
+    // eslint-disable-next-line no-console
+    console.error(err.stack || err.message)
   }
 
   // only show the notification if it finished.
   if (typeof npm.updateNotification === 'string') {
+    // TODO: use proc-log to send `force: true` along with event
     npm.forceLog('notice', '', npm.updateNotification)
   }
 
@@ -204,6 +199,7 @@ const exitHandler = err => {
   }
 
   if (hasLoadedNpm) {
+    // TODO: use proc-log.output.flush() once it exists
     npm.flushOutput(jsonError)
   }
 

lib/utils/format.js

@@ -47,6 +47,4 @@ const formatWithOptions = ({ prefix: prefixes = [], eol = '\n', ...options }, ..
   return lines.reduce((acc, l) => `${acc}${prefix}${prefix && l ? ' ' : ''}${l}${eol}`, '')
 }
 
-const format = (...args) => formatWithOptions({}, ...args)
-
-module.exports = { format, formatWithOptions }
+module.exports = { formatWithOptions }