feat: add spinner #7432
feat: add spinner #7432
Conversation
lukekarrys
force-pushed
the
lk/spinner
branch
18 times, most recently
from
a38b050
to
ccd4fd0
Compare
lukekarrys
force-pushed
the
lk/spinner
branch
6 times, most recently
from
550db9b
to
7747e03
Compare
| @@ -527,7 +526,6 @@ graph LR; | |||
| npm-->pacote; | |||
| npm-->parse-conflict-json; | |||
| npm-->proc-log; | |||
| npm-->proggy; | |||
There was a problem hiding this comment.
proggy was previously required in display.js as a placeholder for how progress messages could be rendered, but not actually used. I opted to remove it as we now are showing a spinner without any specific messages. A future iteration that should useful progress messages would bring this dependency back.
|
|
||
| // clear the prompt line | ||
| output.standard('') | ||
|
|
There was a problem hiding this comment.
This is no longer needed because we clear the prompt line in display.js. This is good because currently we wouldn't clear the input in cases besides the emitter's abort event.
Closes #7425
| this.#enabled = enabled | ||
| this.#spinner = unicode ? Progress.dots : Progress.lines | ||
| // Dont render the spinner for short durations | ||
| this.#render(200) |
There was a problem hiding this comment.
What's the performance cost here? Is 500 still readable but measurably faster? Ultimately if performance is that important folks should turn this off but ... a sensible default should be something that comes from a place of some testing.
There was a problem hiding this comment.
I don't think it should affect performance. This delay is more for UI. A command that only takes 100ms shouldn't attempt to render the spinner since it won't appear as more than a flash of a spinner. I landed on 200ms as the "if nothing has been output yet, let the user know that things are happening". But this number should be very open to changing in the future.
There was a problem hiding this comment.
Reading the next comment, I see what you're talking about with the interval frame rate. That number is located in the Progress.dots.duration and Progress.lines.duration and we should also feel free to experiment with those as well.
|
@lukekarrys @wraithgar Thanks for taking the time to add the spinner! However, this could be improved upon, compared to how it was before in 10.5.2. My concerns are the following:
Wouldn't that be nicer? |
Measured time with
This. We need more verbosity. |
Closes #7425
This is a little hard to test because everything should continue to work without progress, as evidenced by the lack of churn in the tests and snapshots here. The only existing tests that changed are the addition of newlines after prompts which is new behavior as part of this PR.
I resorted to manually running some commands to get an idea of how the various states worked together:
node . exec -- npm@4This should show progress at the start and then hide it as it prompts the user to install
npm@4.Ctrl+Cshould exit from the prompt and the error should display on the next line (this is a current bug).node . audit signatures --loglevel=httpThis should show a lot of http log messages while always keeping the spinner on the last line of output. The spinner also should not jump between frames regardless of how quickly the log messages show up.
node . packThis should immediately show the banners and output from
prepackand not show the spinner until the actual packing is happening.node . loginThe spinner should never while the prompt is being displayed.
node . view npmThe spinner should appear while data is being fetched and then disappear for the rest of the command as output is being displayed. The end output on completion should not have any spinner frames rendered on new lines in the output. The old progress bar achieved this by calling
npmlog.disableProgress()but now it is able to hide the spinner appropriately even while outputting individual lines to stdout.