Catch Promise rejection on SIGINT

[highvoltag3]

Open to feedback fixes #88

[changeset-bot]

🦋 Changeset is good to go

Latest commit: ed95a23

We got this.

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

[codecov-io]

Codecov Report

Merging #92 into master will decrease coverage by 0.61%.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   83.23%   82.61%   -0.62%     
==========================================
  Files          31       31              
  Lines         811      817       +6     
  Branches      144      145       +1     
==========================================
  Hits          675      675              
- Misses        125      131       +6     
  Partials       11       11

Impacted Files	Coverage Δ
packages/cli/src/utils/cli.ts	10% <9.09%> (+2.3%)	⬆️
packages/get-workspaces/src/index.ts	96.29% <0%> (ø)	⬆️
.../src/utils/bolt-replacements/getDependentsGraph.ts
.../src/utils/bolt-replacements/getDependencyGraph.ts
...i/src/utils/bolt-replacements/getDependencyInfo.ts
...utils/bolt-replacements/versionRangeToRangeType.ts
.../src/utils/bolt-replacements/getDependentsGraph.js	100% <0%> (ø)
.../src/utils/bolt-replacements/getDependencyGraph.js	90.62% <0%> (ø)
...i/src/utils/bolt-replacements/getDependencyInfo.js	100% <0%> (ø)
...utils/bolt-replacements/versionRangeToRangeType.js	100% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4bbab4...ed95a23. Read the comment docs.

[emmatown]

🎉

[emmatown]

I don't think we use askList anywhere now so we should probably remove it.

[highvoltag3]

Yeah, I noticed but felt like I should cover it in case it was meant to be used in the future... I'll remove it though.

[Noviny]

Thanks for this! I don't know if we need to log anything in this situation, as most CLIs just exit silently.

Second thing, can we check the type of error and re-throw for errors other than manual process exits to make sure we don't swallow genuine errors.

[highvoltag3]

Thanks for this! I don't know if we need to log anything in this situation, as most CLIs just exit silently.

Second thing, can we check the type of error and re-throw for errors other than manual process exits to make sure we don't swallow genuine errors.

@Noviny sure, I'll make the change.

Regarding exiting silently vs showing a message In my mind, it's good to provide the user with some feedback even if it's redundant (say I know I need to pick a package from the checklist but I want to exit now and hit CTRL + C) re-iterating that a selection is needed or a question needs to be answered kind of sticks to the back of my head if I re-run the command again and go through the flow.

That being said, happy to do it either way. I'll wait for your response before moving forward.

[highvoltag3]

Thinking about this a bit more, I think I digressed...

Thanks for this! I don't know if we need to log anything in this situation, as most CLIs just exit silently.

IMHO I think it's worth giving the user some context when they are exiting in the middle of the flow (specifically in a step, like answering a question) even though they are quitting purposely. (So, I get why you're saying it) either way is fine with me. :)

Second thing, can we check the type of error and re-throw for errors other than manual process exits to make sure we don't swallow genuine errors.

Technically this was already happening, I didn't actually plug into the Ctrl + C (SIGINT) event, rather I addressed the errors you were seeing (and the ones that popped whenever quitting the flow) which were basically that the promises building the prompts/questions didn't have a .catch() so ctrl + c'ing out of them threw an error because the execution continued to the next question and since it depends on the previous answers it would 💩

[Noviny]

IMHO I think it's worth giving the user some context

Possibly some context line is good, but the context seems more like process manually exited - no changeset written. Please choose at least one item suggests the process failed because they did not make a valid selection, which is not what this is an error for. Giving a message about what they should have done suggests that they made a mistake, which is not what we want to communicate.

If we wanted to let them know what step they exited on, we could go with:

Process manually exited while selecting packages to bump

etc, but that can be seen already in the terminal.

This is also why I want to detect for SIGINT, as it's the only known error and just rethrow other things. The way this is set up, it will display this message for any error that occurs during steps, and if there is a bug that causes the process to crash, it will hide that, and return this message that implies it's the user's fault (in how I am reading this).

[highvoltag3]

Fair enough, I'll remove the .catch() for the promises in the flow and intercept SIGINT it should be "easier" just need to figure out how to avoid Enquirer from catching CTRL+C or bubbling it up to process

[highvoltag3]

@Noviny Should be all good for another review :)