8000 fix: the spinner by mbtools · Pull Request #8322 · npm/cli · GitHub
[go: up one dir, main page]
Skip to content

fix: the spinner #8322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 13 commits into
base: latest
Choose a base branch
from
Open

fix: the spinner #8322

wants to merge 13 commits into from

Conversation

mbtools
Copy link
Contributor
@mbtools mbtools commented May 24, 2025 •
edited
< 8000 span class="sr-only">Loading

TL;DR

I fixed the spinner, yay! If you like it, buy me a ☕ or 🍕 .

Background

In 10.7.0, a spinner was introduced to the npm cli, a small progress indicator showing that npm is still busy and not dead.

I'm not a JavaScript developer, but I love a good challenge: Debugging and finding a fix for annoying bugs that no one had time to or wanted to fix is as good as sx (well, almost).

Problem

With the spinner came a regression that disrupted the user experience when npm asked for input during login or adding a user: The spinner would show up after the second prompt and drive users nuts.

For example, the username prompt for npm adduser would be ok but subsequent prompts for password and email showed the spinner although users were asked to enter something. Very confusing.

Cursor_2354

Root Cause

The was a race condition between turning off the spinner and showing a new prompt.

I tried setting timers, waiting for the next tick, and a flag indicating if there's an active prompt. None worked.

Solution

This PR improves the cli prompt experience by ensuring the progress spinner does not appear between consecutive user prompts (such as username and password).

  • Fixes race condition in input handler
  • Avoids spinner interfering with input prompts
  • Properly adds newlines with silent prompts or when input is cancelled by user

Demo

Here's a demo of what it looks after this PR 😃 (uses Verdaccio in case you wonder)

Cursor_2345

References

Ref #7432
Closes #7425
Closes #7583

Enjoy, Marc

@mbtools mbtools marked this pull request as ready for review May 25, 2025 18:02
@mbtools mbtools requested a review from a team as a code owner May 25, 2025 18:02
wraithgar
wraithgar reviewed May 27, 2025
View reviewed changes
wraithgar
wraithgar reviewed May 27, 2025
View reviewed changes
wraithgar
wraithgar reviewed May 27, 2025
View reviewed changes
@mbtools
Copy link
Contributor Author
mbtools commented Jun 3, 2025 •
edited
Loading

My AI friends and I spend a long time to figure this out, but we found a solution that is straight forward.

Details

Here are the input events as they are processed currently, for example with npm adduser:

input.KEYS.read
input.KEYS.start
Username: me
input.KEYS.read.finally
input.KEYS.read
input.KEYS.start
Password: input.KEYS.end
input.KEYS
96A8
.read 
input.KEYS.start
Email (this will be public): input.KEYS.read.finally
input.KEYS.end
me@me.com
input.KEYS.read.finally
input.KEYS.end

When input.KEYS.read is processed, it calls input.start() which immediately emits input.KEYS.start. However, the corresponding input.KEYS.end is only emitted when the promise returned by input.start() settles (due to the .finally() mechanism in proc-log input.start function).

This creates a race condition where:

  • The next prompt's input.KEYS.read and input.KEYS.start are emitted synchronously
  • The previous prompt's input.KEYS.end is emitted asynchronously when the promise chain completes

This explains why we see the events out of order:

  • input.KEYS.read (username)
  • input.KEYS.start (username)
  • input.KEYS.read.finally (username)
  • input.KEYS.read (password) - next prompt starts
  • input.KEYS.start (password) - next prompt starts
  • input.KEYS.end (username) - previous prompt finally ends

The issue is that the input.KEYS.end event is tied to the promise resolution timing rather than being emitted immediately when the prompt completes, causing the overlapping event sequence.

Solution

We switched to sequential input management because:

  • Minimal changes: Only requires modifying the display handler
  • Clear event ordering: Ensures start/end events are properly sequenced
  • Backward compatible: Doesn't change the external API
  • Simple to understand: The flow is straightforward and predictable

The key insight is to emit the input.KEYS.end event immediately when the input promise resolves, rather than relying on proc-log's finally callback mechanism. This ensures that each input operation completes fully before the next one begins. Therefore, the prompt counter is not required anymore.

Cancelled user prompts are now handled directly in catch making finally obsolete.

As suggested, we're now passing the silent prompt parameter to the inputHandler. This way we can output a newline for password prompts, so they are not overwritten by subsequent output.

Summary

  • Fixes race condition in input handler
  • Avoids spinner interfering with input prompts
  • Properly adds newlines with silent prompts or when input is cancelled by user
  • Added tests to cover various prompt cases

😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wraithgar wraithgar wraithgar left review comments

@lukekarrys lukekarrys lukekarrys left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] npm shows spinner while waiting for password [BUG] npm 10.6.0 doesn't log anything while npm install
3 participants
@mbtools @wraithgar @lukekarrys
0