fix: progress bar code cleanup

 - inline this.#render logic.  The initial setTimeout is only used during load(), so run it there instead.
 - .unref() the progress bar's recurring timeout object
 - reuse the progress bar's timeout object with .refresh() instead of making a new one every frame
 - consolidate comments into one line and remove stale comments
 - skip clearing the line on the first frame when resuming the spinner
 - removed the test from $8631 as it was only testing that the very first frame of the spinner didn't clear the line, not any subsequent frames

This is built off of #8631, moving the "skip clearing the line..." logic into a flag that's passed during .resume()

Author: wraithgar

lib/utils/display.js

@@ -216,13 +216,11 @@ class Display {
     timing,
     unicode,
   }) {
-    // get createSupportsColor from chalk directly if this lands
-    // https://github.com/chalk/chalk/pull/600
     const [{ Chalk }, { createSupportsColor }] = await Promise.all([
       import('chalk'),
       import('supports-color'),
     ])
-    // we get the chalk level based on a null stream meaning chalk will only use what it knows about the environment to get color support since we already determined in our definitions that we want to show colors.
+    // We get the chalk level based on a null stream, meaning chalk will only use what it knows about the environment to get color support since we already determined in our definitions that we want to show colors.
     const level = Math.max(createSupportsColor(null).level, 1)
     this.#noColorChalk = new Chalk({ level: 0 })
     this.#stdoutColor = stdoutColor
@@ -307,14 +305,14 @@ class Display {
         if (this.#outputState.buffering) {
           this.#outputState.buffer.push([level, meta, ...args])
         } else {
-          // HACK: Check if the argument looks like a run-script banner. This can be replaced with proc-log.META in @npmcli/run-script
+          // XXX: Check if the argument looks like a run-script banner.  This should be replaced with proc-log.META in @npmcli/run-script
           if (typeof args[0] === 'string' && args[0].startsWith('\n> ') && args[0].endsWith('\n')) {
             if (this.#silent || ['exec', 'explore'].includes(this.#command)) {
               // Silent mode and some specific commands always hide run script banners
               break
             } else if (this.#json) {
               // In json mode, change output to stderr since we don't want to break json parsing on stdout if the user is piping to jq or something.
-              // XXX: in a future (breaking?) change it might make sense for run-script to always output these banners with proc-log.output.error if we think they align closer with "logging" instead of "output"
+              // XXX: in a future (breaking?) change it might make sense for run-script to always output these banners with proc-log.output.error if we think they align closer with "logging" instead of "output".
               level = output.KEYS.error
             }
           }
@@ -339,12 +337,12 @@ class Display {
         break
 
       case input.KEYS.read: {
-        // The convention when calling input.read is to pass in a single fn that returns the promise to await. resolve and reject are provided by proc-log
+        // The convention when calling input.read is to pass in a single fn that returns the promise to await. resolve and reject are provided by proc-log.
         const [res, rej, p] = args
         return input.start(() => p()
           .then(res)
           .catch(rej)
-          // Any call to procLog.input.read will render a prompt to the user, so we always add a single newline of output to stdout to move the cursor to the next line
+          // Any call to procLog.input.read will render a prompt to the user, so we always add a single newline of output to stdout to move the cursor to the next line.
           .finally(() => output.standard('')))
       }
     }
@@ -419,17 +417,17 @@ class Progress {
   static dots = { duration: 80, frames: ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏'] }
   static lines = { duration: 130, frames: ['-', '\\', '|', '/'] }
 
-  #stream
-  #spinner
   #enabled = false
-
   #frameIndex = 0
-  #lastUpdate = 0
   #interval
+  #lastUpdate = 0
+  #spinner
+  #stream
+  // Initial timeout to wait to start rendering
   #timeout
   #rendered = false
 
-  // We are rendering is enabled option is set and we are not waiting for the render timeout
+  // We are rendering if enabled option is set and we are not waiting for the render timeout
   get #rendering () {
     return this.#enabled && !this.#timeout
   }
@@ -446,8 +444,13 @@ class Progress {
   load ({ enabled, unicode }) {
     this.#enabled = enabled
     this.#spinner = unicode ? Progress.dots : Progress.lines
-    // Dont render the spinner for short durations
-    this.#render(200)
+    // Wait 200 ms so we don't render the spinner for short durations
+    this.#timeout = setTimeout(() => {
+      this.#timeout = null
+      this.#render()
+    }, 200)
+    // Make sure this timeout does not keep the process open
+    this.#timeout.unref()
   }
 
   off () {
@@ -464,7 +467,7 @@ class Progress {
   }
 
   resume () {
-    this.#render()
+    this.#render(true)
   }
 
   // If we are currently rendering the spinner we clear it before writing our line and then re-render the spinner after.
@@ -479,38 +482,31 @@ class Progress {
     }
   }
 
-  #render (ms) {
-    if (ms) {
-      this.#timeout = setTimeout(() => {
-        this.#timeout = null
-        this.#renderSpinner()
-      }, ms)
-      // Make sure this timeout does not keep the process open
-      this.#timeout.unref()
-    } else {
-      this.#renderSpinner()
-    }
-  }
-
-  #renderSpinner () {
+  #render (resuming) {
     if (!this.#rendering) {
       return
     }
     // We always attempt to render immediately but we only request to move to the next frame if it has been longer than our spinner frame duration since our last update
-    this.#renderFrame(Date.now() - this.#lastUpdate >= this.#spinner.duration)
-    clearInterval(this.#interval)
-    this.#interval = setInterval(() => this.#renderFrame(true), this.#spinner.duration)
+    this.#renderFrame(Date.now() - this.#lastUpdate >= this.#spinner.duration, resuming)
+    if (!this.#interval) {
+      this.#interval = setInterval(() => this.#renderFrame(true), this.#spinner.duration)
+      // Make sure this timeout does not keep the process open
+      this.#interval.unref()
+    }
+    this.#interval.refresh()
   }
 
-  #renderFrame (next) {
+  #renderFrame (next, resuming) {
     if (next) {
       this.#lastUpdate = Date.now()
       this.#frameIndex++
       if (this.#frameIndex >= this.#spinner.frames.length) {
         this.#frameIndex = 0
       }
     }
-    this.#clearSpinner()
+    if (!resuming) {
+      this.#clearSpinner()
+    }
     this.#stream.write(this.#spinner.frames[this.#frameIndex])
     this.#rendered = true
   }

test/lib/utils/display.js

@@ -29,7 +29,6 @@ const mockDisplay = async (t, { mocks, load } = {}) => {
     ...procLog,
     display,
     displayLoad,
-    streams: logs.streams,
     ...logs.logs,
   }
 }
@@ -102,50 +101,6 @@ t.test('can do progress', async (t) => {
   t.strictSame(outputs, ['before input', 'during input', 'after input'])
 })
 
-t.test('progress resume does not clear output when spinner inactive', async (t) => {
-  const { input, output, outputs, streams } = await mockDisplay(t, {
-    load: {
-      progress: true,
-    },
-  })
-
-  const origClearLine = streams.stderr.clearLine
-  const origCursorTo = streams.stderr.cursorTo
-  let clearCalls = 0
-  let cursorCalls = 0
-  streams.stderr.clearLine = (...args) => {
-    clearCalls++
-    return origClearLine.call(streams.stderr, ...args)
-  }
-  streams.stderr.cursorTo = (...args) => {
-    cursorCalls++
-    return origCursorTo.call(streams.stderr, ...args)
-  }
-
-  t.teardown(() => {
-    streams.stderr.clearLine = origClearLine
-    streams.stderr.cursorTo = origCursorTo
-  })
-
-  // ensure the spinner has rendered at least once so progress.off clears it
-  await timers.setTimeout(300)
-
-  const endInput = input.start()
-  await timers.setTimeout(0)
-
-  clearCalls = 0
-  cursorCalls = 0
-
-  output.standard('no trailing newline')
-
-  endInput()
-  await timers.setTimeout(0)
-
-  t.equal(clearCalls, 0, 'resume does not clear the line when spinner inactive')
-  t.equal(cursorCalls, 0, 'resume does not reposition cursor when spinner inactive')
-  t.equal(outputs.at(-1), 'no trailing newline', 'output remains visible')
-})
-
 t.test('handles log throwing', async (t) => {
   class ThrowInspect {
     #crashes = 0;