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

feat(@clack/prompts): adapt spinner to CI environment #169

Merged
merged 13 commits into from
Dec 14, 2024

Conversation

orochaa
Copy link
Contributor

@orochaa orochaa commented Sep 22, 2023

This PR addresses a issue reported in GitHub Actions where spinner was excessively writing messages, leading to confusion and cluttered output. To enhance the CI workflow and provide a smoother experience, the following changes have been made only for CI environment:

  • Messages will now only be written when a spinner method is called and the message updated, preventing unnecessary message repetition.
  • There will be no loading dots animation, instead it will be always ...
  • Instead of erase the previous message, action that is blocked during CI, it will just write a new one.

Demo

2023-09-22_17-06-00.mov

Closes #168

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2023

🦋 Changeset detected

Latest commit: 38c5371

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clack/prompts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ulken
Copy link
Collaborator

ulken commented Sep 23, 2023

Suggestion: use something like https://www.npmjs.com/package/ci-info to cover more CI environments.

@orochaa orochaa force-pushed the static-ci branch 2 times, most recently from 5d9d06d to 252de51 Compare November 26, 2024 13:40
examples/basic/package.json Outdated Show resolved Hide resolved
antony
antony previously requested changes Dec 3, 2024
Copy link

@antony antony left a comment

Choose a reason for hiding this comment

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

Suggest that the new std-env dependency is removed as it probably isn't required. Generally, CI systems set CI=true, and I've yet to encounter one which doesn't.

If some random CI system doesn't set CI=true, then it's trivial for a user to just set it.

But in addition, if somebody wanted the old behaviour in CI, they could override it using this variable - whereas the std-env library checks a whole pile of other variables, making that hard to do.

In addition,

@orochaa
Copy link
Contributor Author

orochaa commented Dec 3, 2024

Hey @antony, thanks for the review!

I agree with the suggestions and have applied then to the PR, now we just check if CI variable is set to true. This way we cover most CI environments and still keep the bundle size small.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is great 💜

.changeset/thin-moose-tease.md Outdated Show resolved Hide resolved
@natemoo-re natemoo-re dismissed antony’s stale review December 14, 2024 05:56

changes have been made

@natemoo-re natemoo-re merged commit f9f139d into bombshell-dev:main Dec 14, 2024
3 checks passed
@orochaa orochaa deleted the static-ci branch December 15, 2024 04:34
@mdjastrzebski
Copy link

@natemoo-re when do you plan to release this fix?

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.

[Request] Disable continuous terminal rewriting in CI
5 participants