Skip to content

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

Open
zemccartney wants to merge 4 commits into
masterfrom
handle-sigint-npm-init
Open

Prevent bin process from crashing on sigint when sent during npm init dialog#55
zemccartney wants to merge 4 commits into
masterfrom
handle-sigint-npm-init

Conversation

@zemccartney

@zemccartney zemccartney commented May 4, 2021

Copy link
Copy Markdown
Member

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

coveralls commented May 4, 2021

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@zemccartney

zemccartney commented May 4, 2021

Copy link
Copy Markdown
Member Author

@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

devinivy commented Dec 7, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
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