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(@clack/prompts): add spinner hook to handle process exit #113

Merged
merged 12 commits into from
Aug 9, 2023

Conversation

Mist3rBru
Copy link
Contributor

@Mist3rBru Mist3rBru commented Mar 29, 2023

Closes #106

It adds a hook to spinner, that prints a default message and avoid terminal conflicts, it triggers on:

  • Uncaught code exception
  • Unhandled promise rejection
  • Ctrl + C
  • Kill process
  • Process shutdown

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2023

🦋 Changeset detected

Latest commit: 4a5a00f

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

@Mist3rBru Mist3rBru changed the title feat(#106): spinner hook to handle process exit fix(#106): add spinner hook to handle process exit Mar 29, 2023
@Mist3rBru
Copy link
Contributor Author

Code with try-catch logic, will still be able to call spinner.stop() and print their message

@cpreston321 cpreston321 requested a review from ulken April 4, 2023 01:49
@cpreston321 cpreston321 changed the title fix(#106): add spinner hook to handle process exit fix(@clack/prompts): add spinner hook to handle process exit Aug 9, 2023
Copy link
Collaborator

@cpreston321 cpreston321 left a comment

Choose a reason for hiding this comment

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

@Mist3rBru The changes look good!

Thanks for fixing the conflicts!

@cpreston321
Copy link
Collaborator

Hey @Mist3rBru

While testing I did see this:

image

If you want to test yourself: cd examples/basic && pnpm start

The code looks good but it seems that maybe something messed up when merge. I just did a CRTL+C when installing deps was showing.

Thanks,
CP 🚀

@cpreston321 cpreston321 self-requested a review August 9, 2023 16:25
Copy link
Collaborator

@cpreston321 cpreston321 left a comment

Choose a reason for hiding this comment

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

I left some comments above based on testing.

@cpreston321 cpreston321 self-requested a review August 9, 2023 16:54
Copy link
Collaborator

@cpreston321 cpreston321 left a comment

Choose a reason for hiding this comment

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

I went ahead and fixed it the bug!

@cpreston321 cpreston321 merged commit 9ce1295 into bombshell-dev:main Aug 9, 2023
2 checks passed
@Mist3rBru Mist3rBru deleted the fix/spinner-exit branch August 25, 2023 18:57
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] Spinner to handle cleaning up on process exit
2 participants