Skip to content

Conversation

BYK
Copy link
Member

@BYK BYK commented Aug 3, 2017

Summary

Fixes #4023. There was nothing preventing the console reporter from
having more than one progress bars, or keep updating and rendering
its progress bar after it was finished or the reporter was "done".

This patch stores the active progress bar, and stops it before
footer is printed out. Also makes sure the progress bar itself
ignores any updates once it stops.

Test plan

One, weak, existing test with updated snapshot. Manual steps:

  • Run yarn install express
  • Observe that your console is properly cleared after yarn
    finishes and the last thing you see is the "Done in X.YZs."
    message and not a corrupted progress bar.

**Summary**

Fixes #4023. There was nothing preventing the console reporter from
having more than one progress bars, or keep updating and rendering
its progress bar after it was finished or the reporter was "done".

This patch stores the active progress bar, and stops it before
`footer` is printed out. Also makes sure the progress bar itself
ignores any updates once it stops.

**Test plan**

One, weak, existing test with updated snapshot. Manual steps:

- Run `yarn install express`
- Observe that your console is properly cleared after `yarn`
  finishes and the last thing you see is the "Done in X.YZs."
  message and not a corrupted progress bar.
@BYK BYK requested a review from arcanis August 3, 2017 12:31
@mjesun mjesun self-requested a review August 4, 2017 10:34
chars: [string, string];
delay: number;
id: ?number;
_callback: ?Function;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you properly type this callback function?

if (!this.id) {
clearTimeout(this.id);
clearLine(this.stdout);
this.id = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If !this.id then the timeout is cleared?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

@BYK BYK merged commit 280b6eb into master Aug 4, 2017
@BYK BYK deleted the progress branch August 4, 2017 13:33
kaylie-alexa pushed a commit to kaylie-alexa/yarn that referenced this pull request Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants