Skip to content

Tracking issue for breaking changes for ProgressBar #6501

Open

Activity

kt3k

kt3k commented on Mar 21, 2025

@kt3k
MemberAuthor

I think we first need to settle on #6407 to start landing these changes

BlackAsLight

BlackAsLight commented on Mar 21, 2025

@BlackAsLight
Contributor

On a somewhat related, somewhat different direction, both ProgressBar and Spinner have a flaw in their design in two ways.

  1. You can't print other things to the console at the same time without it becoming a mess.
  2. Their existence as a class gives the impression that you can have multiple running at a time, but doing so would make a mess of the terminal as they all try and overwrite each other.

I think their design needs to be reworked entirely to resolve these issues. I believe the first issue can be fixed by using some different terminal control sequences to move the cursor for rewrites instead of clearing the current line before writing again. For the second issue, I think it would be better if the APIs acted as if they were controlling some global state and not creating a piece of memory to be passed around. With this global state type of design, we could also allow more complex outputs in the terminal like multiple progress bars/ spinners or possibly even getting user input while a progress bar also updates on screen.

timreichen

timreichen commented on Mar 21, 2025

@timreichen
Contributor

A few other things I noticed:

  • ProgressBarOptions.fmt is an abbreviation which is not very web-apish. Maybe it should be called something like formatter or format instead.
  • ProgressBarFormatter seems overly complex atm. styledTime and styledData seem like they could be getters instead of functions.
  • ProgressBarFormatter progressBar, styledTime and styledData should be strings without a trailing whitespaces.
kt3k

kt3k commented on Apr 28, 2025

@kt3k
MemberAuthor

It seems difficult to reach agreement in #6407 and #6408

I'm considering to land only the first 3 (#6406, #6409, #6430) for now.

timreichen

timreichen commented on Apr 28, 2025

@timreichen
Contributor

I have a radical proposal that might solve all these issues:
Let's rewrite parts of ProgressBar and ProgressBarStream:

  • make ProgressBar sync and let it write to typeof Deno.stderr | typeof Deno.stdout instead of WritableStreamDefaultWriter, like Spinner does.
  • add start() only to ProgressBar like in Spinner.
  • make ProgressBar not use Units as a default value and style, because this is a very specialized case for downloads/streams. Rather make it a progress bar with 1 as a default max.

We also should check the behavior of ProgressBarStream.
As of now we can do

new ProgressBarStream(Deno.stdout.writable, { max: 1 })

This will create a progress bar that cannot be stopped.

WDYT?

kt3k

kt3k commented on Apr 30, 2025

@kt3k
MemberAuthor

make ProgressBar sync and let it write to typeof Deno.stderr | typeof Deno.stdout instead of WritableStreamDefaultWriter, like Spinner does.

I'm neutral about this. This change probably makes the implementation simpler, but also makes the stream version a bit less efficient because of blocking writes.

add start() only to ProgressBar like in Spinner.

I'm against this change as stated in #6408. This introduces many edge case scenarios like restarting it after ended. That make implementation and testing unnecessarily complex.

It looks like this suggestion is only based on 'alignment' to web platform api (SpeechRecognition.start), but that argument seems to have counterexamples (WebSocket and Worker classes start main function at constructors)

make ProgressBar not use Units as a default value and style, because this is a very specialized case for downloads/streams. Rather make it a progress bar with 1 as a default max.

Can you give example of progress bars which ends with max value 1? (In my view progress bar ranges are typically 0 to 100%, 0 to [MAX] bytes/files/whatever. I don't remember progress bar ending with max = 1).

kt3k

kt3k commented on May 20, 2025

@kt3k
MemberAuthor

I'm going to land the first 3

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @kt3k@timreichen@BlackAsLight

        Issue actions

          Tracking issue for breaking changes for ProgressBar · Issue #6501 · denoland/std