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

Add option to not show progress animations in logs #942

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

garrettjstevens
Copy link

Fixes #689.

Adds a --noProgress flag (based on lerna's --no-progress, but made camelCase to match existing flags). When used, the progress-estimator logger is replaced by a simple console.log of the message. The progress animations will also be removed when the environment is not a TTY and when running in CI.

Thanks for a great tool, and let me know if there's anything I can do to improve this PR.

Add --noProgress flag that replaces the progress animations in the log
with a simple log of the message. The progress animations will also be
removed when the environment is not a TTY and when running in CI.
@vercel

This comment has been minimized.

Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

To clarify, this is not a full, formal review, and normally I would not review until it is full, but there's a handful of issues (and improvements) with the PR as is:

  • Docs were not updated
  • The comment in SharedOptions is not correct (changing noProgress in the config does not change it, so the word "Should" is confusing and potentially misleading; "Is output progress being shown?" would be better)
  • Only 1 command was updated (build and watch should both be updated)
  • The isTTY portion is a breaking change and therefore can only go out in a breaking release. I would recommend that part be separated into a different PR as such.
  • I think this check should be embedded into createProgressEstimator instead of in individual uses. That will also make it useful when other functions use createProgressEstimator
  • Typings could be improved/more specific (Promise<any> instead of unknown)
  • Is there a way to infer Lerna's current progress output status? That way there would not necessarily be a need for a new flag, which is what I was hoping to avoid in the issue

@garrettjstevens
Copy link
Author

Thanks @agilgur5, I'll work on addressing those issues.

@garrettjstevens
Copy link
Author

A few comments on my last commit:

  • The comment in SharedOptions is not correct (changing noProgress in the config does not change it, so the word "Should" is confusing and potentially misleading; "Is output progress being shown?" would be better)

Since the option is a negative, I went with "Are output progress animations disabled?". Is that ok?

  • Only 1 command was updated (build and watch should both be updated)

I've added the option to watch as well. I did this by using the isEnabled option of the ora spinner.

  • The isTTY portion is a breaking change and therefore can only go out in a breaking release. I would recommend that part be separated into a different PR as such.

It's now removed from this PR. I'll open a draft PR with that change separately.

  • Is there a way to infer Lerna's current progress output status? That way there would not necessarily be a need for a new flag, which is what I was hoping to avoid in the issue

In my use case I'm not using Lerna, so I'm hoping we can add this new flag.

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.

disable progress-estimator logging (for lerna run --stream build or CI)
3 participants