feat(cli/unstable): add ProgressBar start() method

[timreichen]

This PR adds start() method to ProgressBar. This in order to align to Spinner and to allow instantiation of ProgressBar before usage.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 96.03%. Comparing base (360851f) to head (469f588).

Files with missing lines	Patch %	Lines
cli/unstable_progress_bar.ts	73.91%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6408      +/-   ##
==========================================
- Coverage   96.03%   96.03%   -0.01%     
==========================================
  Files         562      562              
  Lines       42418    42431      +13     
  Branches     6387     6390       +3     
==========================================
+ Hits        40738    40748      +10     
- Misses       1641     1644       +3     
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BlackAsLight]

The start method should allow for the options to be optionally provided again in case the user wants to change values, like the max.

This allows it to be reused later down the line.

[timreichen]

The start method should allow for the options to be optionally provided again in case the user wants to change values, like the max.

This allows it to be reused later down the line.

Instead of allowing start() to pass another options object, I think it is more web api-ish to have all properties public that can be changed while progressBar is running or when it is reused. Something along the lines of

const progressBar = new ProgressBar()
progressBar.start()
...
progressBar.stop()

...
progressBar.max = 100
progressBar.start()
...

The other benefit of that is that a public value property makes add() obsolete because it can be done with progressBar.value += increment. It is also more flexible be cause the value can be set directly progressBar.value = newValue.

WDYT?

[BlackAsLight]

I think some properties like value, max, barLength and fmt could make sense, but other properties like clear and keepOpen could break things/ have no effect.

Looking at the current state of the code even though one can call start and stop multiple times, the progress bar can't be reused. The interval ID isn't cleared for a second round and the writer is closed when stop is called, possibly closing the writable passed in.

[kt3k]

I'm not in favor of this change as the added complexity is not small, but this doesn't support any new practical scenario.

[timreichen]

I'm not in favor of this change as the added complexity is not small, but this doesn't support any new practical scenario.

Do you mean the addition of start() or public properties?

I would argue that there is a small complexity addition for the first if the progress bar resets when calling start() after stop() again. We can takle the second in another PR.

[kt3k]

As @BlackAsLight pointed, this introduces a new bugs related to start method. Because end method doesn't reset this.#intervalId to undefined, the second start call doesn't print the progress anymore. Also if the user passed keepOption: false, then the #writer.close() call in end() closes the writable, and the user gets the uncaught error in #print function after the 2nd start call. These need to be fixed with new test cases. I would see them fair amount of added complexity. If we don't support start method, those scenarios don't exist.

[timreichen]

As @BlackAsLight pointed, this introduces a new bugs related to start method. Because end method doesn't reset this.#intervalId to undefined, the second start call doesn't print the progress anymore. Also if the user passed keepOption: false, then the #writer.close() call in end() closes the writable, and the user gets the uncaught error in #print function after the 2nd start call. These need to be fixed with new test cases. I would see them fair amount of added complexity. If we don't support start method, those scenarios don't exist.

I'll have another look after some of the other PRs have landed. I imagine some of the mentioned issues add not too much complexity, some are even already implemented in Spinner. I think to have a start() method is worth it not least because lots of web apis use the convention of class initialization as a setup and a method to start the process (MutationObserver.observe(), SpeechRecognition.start(), etc.)

[kt3k]

lots of web apis use the convention of class initialization as a setup and a method to start the process (MutationObserver.observe(), SpeechRecognition.start(), etc.)

There are also APIs which start the main processing at the constructor such as WebSocket Worker. I don't think having start is particularly aligned better with Web platform APIs.

[kt3k]

Also alignment to the web platform api shouldn't be the goal in my view. It should be only supplementary for design decisions when there are equally possible designs.

If the alignment to web apis causes additional unnecessary complexity, then I think we should avoid it.