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: add windows signals SIGUSR2 & SIGUSR1 to terminate the process #1938

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

immortalmind2016
Copy link
Contributor

on SIGUSR2 & SIGUSR1 triggers in windows, we should invoke taskkill

@netlify
Copy link

netlify bot commented Oct 16, 2021

✔️ Deploy Preview for nodemon ready!

🔨 Explore the source changes: 7fa1986

🔍 Inspect the deploy log: https://app.netlify.com/sites/nodemon/deploys/616adc1efcf6350007685c2f

😎 Browse the preview: https://deploy-preview-1938--nodemon.netlify.app

@immortalmind2016
Copy link
Contributor Author

immortalmind2016 commented Oct 17, 2021

fix issues: #1937 , #1915 and #1936

@remy
Copy link
Owner

remy commented Oct 18, 2021

Any details on how you're testing this and on how many platforms within windows?

@immortalmind2016
Copy link
Contributor Author

immortalmind2016 commented Oct 18, 2021

@remy
tested inside windows 10 locally and works well.
by
run command
node bin/nodemon -e ts -x ts-node src/index.ts
Expected output
when I changed anything within ts files the process restarted
Actual output
when I changed anything within ts files the process restarted

i saw the timeline of git changes and the bulk of code was changed inside a commit but the one who changed it , forget to add the other signals.

@remy
Copy link
Owner

remy commented Oct 18, 2021

Sorry, I gathered windows. I meant: powershell? Version? WSL? Inside vscode/outside? Command? Etc.

The problems are across many different versions of terminals and since your change doesn't have tests (because the CI doesn't do windows - ie. not your fault) it's hard to know if it will have a positive effect.

Further changes to this area need to be really cross tested a lot since it seems so brittle in the first place.

@immortalmind2016
Copy link
Contributor Author

immortalmind2016 commented Oct 18, 2021

command
node ../projects/forkednodemon/bin/nodemon -e ts -x ts-node src/index.ts

tested across

  • git bash
  • windows terminal
  • cmd
  • inside vscode terminal

CMD version output
Microsoft Windows [Version 10.0.19042.1288]
git bash version output
git version 2.31.1.windows.1

PowerShell version output

Major | Minor | Build | Revision
5 | 1 | 19041 | 1237

dump
node ../projects/forkednodemon/bin/nodemon --dump output
output.txt

Note: i tested it also on another windows platform with different version

@immortalmind2016
Copy link
Contributor Author

i may try to write CI test later for the windows environment to nodemon module but in my weekend

@remy
Copy link
Owner

remy commented Oct 18, 2021

Cheers for confirming. We'll want to get some folks to try out with powershell, but what might be useful is if I publish this as a debug build to npm so those people on the open issues are able to give it a test too - that way there's more coverage of testing 👍

Thanks so much for the input too (I know my asking for state of tests sounds a bit defensive - it's only to get a view on what I'm looking at).

Cheers again.

@immortalmind2016
Copy link
Contributor Author

thanks , waiting 👍

@remy
Copy link
Owner

remy commented Oct 18, 2021

available to test with npm i nodemon@debug

@github-actions
Copy link

🎉 This PR is included in version 2.0.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants