Skip to content

fix: handle SIGINT with prompt #2887

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

fix: handle SIGINT with prompt #2887

wants to merge 4 commits into from

Conversation

snitin315
Copy link
Member

What kind of change does this PR introduce?
fix

Did you add tests for your changes?
WIP
If relevant, did you update the documentation?
NO
Summary

Screenshot 2021-08-11 at 6 49 10 AM

Does this PR introduce a breaking change?
No

Other information
NO

@codecov
Copy link

codecov bot commented Aug 11, 2021 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.93%. Comparing base (77668d7) to head (032291e).
Report is 1231 commits behind head on master.

Files with missing lines Patch % Lines
packages/webpack-cli/lib/utils/prompt.js 90.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2887      +/-   ##
==========================================
- Coverage   94.96%   94.93%   -0.03%     
==========================================
  Files          31       31              
  Lines        1708     1718      +10     
  Branches      486      486              
==========================================
+ Hits         1622     1631       +9     
- Misses         86       87       +1
Files with missing lines Coverage Δ
packages/webpack-cli/lib/utils/prompt.js 95.23% <90.00%> (-4.77%) ⬇️

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

alexander-akait
alexander-akait reviewed Aug 12, 2021
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's add test for this, looks good

rishabh3112
rishabh3112 reviewed Aug 13, 2021
View reviewed changes
Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Looks good. For testing it, you may add a case in serve's smoketest only.

@snitin315
Copy link
Member Author

I am not sure how to mock CTRL + C for tests here.

@alexander-akait
Copy link
Member

Just run process using execa and send SIGINT signal, no need to test CTRL + C, because it can be changed by user

alexander-akait
alexander-akait reviewed Aug 20, 2021
View reviewed changes
test/api/prompt.test.js
});

// TODO: fix for windows
if (isWindows) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it is not good to skip windows test here...

Copy link
Member Author

Choose a reason for hiding this comment

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

I will see if I can test it on a windows machine locally.

@alexander-akait
Copy link
Member

@snitin315 let's rebase, I think it is finished and please don't ignore windows tests, I will fix it if it will have problems

evenstensberg
evenstensberg reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Could you rebase?

evenstensberg
evenstensberg reviewed Nov 10, 2024
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

rebase please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait alexander-akait left review comments

@rishabh3112 rishabh3112 rishabh3112 left review comments

@evenstensberg evenstensberg evenstensberg left review comments

Assignees
No one assigned
Labels
PR: conflict PR: small PR: unreviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@snitin315 @alexander-akait @rishabh3112 @evenstensberg @webpack-bot