Skip to content
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.

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

fix(gatsby-cli): fix timers on progress bar #28684

Merged
merged 6 commits into from
Jan 5, 2021
Merged

fix(gatsby-cli): fix timers on progress bar #28684

merged 6 commits into from
Jan 5, 2021

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Dec 18, 2020

This PR fixes something I've seen for a while now. The progress bar in yurnalist (CI=1) will not show you an ETA or total runtime of the progress bar. It remains zero.

Apparently this is caused by a bug by setting curr when creating the progress bar. See visionmedia/node-progress#81

The workaround is to call bar.tick(n) instead. This will cause an incorrect eta when starting from non-zero but at least it'll show you something, and show you the current runtime.

Looking at the repo, I'm not expecting any more fixes for this package.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 18, 2020
@pvdz pvdz added topic: cli Related to the Gatsby CLI type: bug An issue or pull request relating to a bug in Gatsby and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 18, 2020
pieh
pieh previously approved these changes Dec 18, 2020
@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 18, 2020
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

It works ;) left one suggestion but feel free to to ignore

@@ -93,15 +93,27 @@ export function initializeYurnalistLogger(): void {
activities[action.payload.id] = activity
} else if (action.payload.type === ActivityTypes.Progress) {
const bar = new ProgressBar(
` [:bar] :current/:total :elapsed s :percent ${action.payload.text}`,
` [:bar] :current/:total :elapsed s :rate /s :percent ${action.payload.text}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit (feel free to ignore)

Suggested change
` [:bar] :current/:total :elapsed s :rate /s :percent ${action.payload.text}`,
` [:bar] :current/:total :elapseds :rate/s :percent ${action.payload.text}`,

extra space there make it look a bit weird (like it was something separate) - this change does in fact result in

-[=====-------------------------] 18/100 1.5 s 12 /s 18% test #1
+[=====-------------------------] 18/100 1.5s 12/s 18% test #1

"postfixes" are "attached" to their numbers which to me make it more clear what those are (IMO)

but up to you and feel free to ignore

@gatsbybot gatsbybot merged commit f11adbb into master Jan 5, 2021
@gatsbybot gatsbybot deleted the fixprogtime branch January 5, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: cli Related to the Gatsby CLI type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants