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(cli): ctrl+C no longer kills processes (#11434) #11518

Merged
merged 3 commits into from
Jan 2, 2023

Conversation

kinfuy
Copy link
Contributor

@kinfuy kinfuy commented Dec 29, 2022

Description

The binding of shortcut keys makes Ctrl+C unable to exit the program directly. Although the SIGTERM signal is sent upward, it cannot end when the console has an asynchronous output task. I think Ctrl+C should exit process.exit(1)

Additional context

close #11434


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@kinfuy kinfuy changed the title fix: Ctrl+C no longer kills processes (#11434) fix(cli): Ctrl+C no longer kills processes (#11434) Dec 29, 2022
@ArnaudBarre
Copy link
Member

I don't remember what but it causes issues. I need to look at it a bit closer

@sapphi-red
Copy link
Member

I guess it's because await server.close() won't be called if process.exit() is used.

Co-authored-by: Arnaud Barré <arnaud.barre72@gmail.com>
@ArnaudBarre ArnaudBarre changed the title fix(cli): Ctrl+C no longer kills processes (#11434) fix(cli): ctrl+C no longer kills processes (#11434) Jan 2, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Really neat solution 👍

@liangxiwei
Copy link

stil not working in 4.1.1

@liangxiwei
Copy link

@kinfuy kinfuy

@ArnaudBarre
Copy link
Member

Can you provide a repro?
Just tested with npm-run-all & vite 4.1.1, it works (both yarn & pnpm)

@lincenying
Copy link

lincenying commented Feb 20, 2023

After version 4.1.0, press ctrl+c to stop the service, and the console will output:

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

image

@kinfuy
Copy link
Contributor Author

kinfuy commented Feb 20, 2023

The current processing is that ctrl c directly throws process.exit (1), which is indeed consistent with the behavior of ctrl c's unexpected exit, but this prompt will make people feel sick. Maybe there is a need to find a better way

@ArnaudBarre
Copy link
Member

Yeah I was annoyed by this extra log but with the team we decided to go in that direction so everything is supported (shortcuts, running via npm-run-all) without configuration.

The only other solution would be to allow people to out-out of shortcuts bindings. But that's one more thing to document.
For now you can use the JS API is really you don't want this behaviour

futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Co-authored-by: Arnaud Barré <arnaud.barre72@gmail.com>
close vitejs#11434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl+C no longer kills processes running in parallel with vite
7 participants