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

Prevent bin process from crashing on sigint when sent during npm init dialog #55

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zemccartney
Copy link
Member

@zemccartney zemccartney commented May 4, 2021

Addresses #42

Now, on starting the npm init dialog, we ignore SIGINT on our main process, so when you press ctrl + C (or however else you fire off a SIGINT signal), as that dialog prompts you to do to quit at anytime, instead of the main process exiting, the SIGINT reaches the npm init child process spawned by the new command, which then exits.

@coveralls
Copy link

coveralls commented May 4, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling f0126c5 on handle-sigint-npm-init into a1a642d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 319584e on handle-sigint-npm-init into a1a642d on master.

@zemccartney
Copy link
Member Author

zemccartney commented May 4, 2021

@devinivy heyo! ran into some funkiness here due to signal-handling (or lack thereof) on Windows. To resolve, I'm thinking

  1. Skip the test that exercises the newly added SIGINT handling on Windows only
  2. Find someone (work, hapi-hour) who has a Windows machine, have them test running the hpal new directly on this branch. I think my changes will actually work when run from a shell i.e. the test failing on Windows isn't actually a problem, but want to confirm

Assuming my changes are even on the right track, how does that plan sound to you?


For background / explanation of the above, I think / hope the following are true:

@devinivy
Copy link
Member

devinivy commented Dec 7, 2021

@zemccartney apologies for neglecting this! I like that plan. But also— I'm down to land this as-is, since it seems like an improvement to me. Do you have any thoughts/guidance on it?

@devinivy devinivy added the bug label Dec 7, 2021
@zemccartney
Copy link
Member Author

@devinivy ah! jeez, yea, I completely spaced here! But I believe someone at my office has a fairly current Windows machine now, I should be able to test this out there, report back. I could shoot to do that this week, which hopefully means just landing this as-is with the assurance that ctrl-c'ing out of the new dialog works as expected on Windows (though could drop in a TODO / peel off a separate issue if any further remediation's needed). Does that work ok?

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

Successfully merging this pull request may close these issues.

3 participants