NPM Script Ctrl/Cmd + C Errors

[jnielson94]

Hi Kent! 👋

Hopefully this is a pretty quick/small fix, as it is related to a recent change in this PR #177, particularly this section:

cross-env/src/index.js

Lines 28 to 31 in eb37984

	proc.on('exit', code => {
	// exit code could be null when OS kills the process(out of memory, etc)
	process.exit(code === null ? 1 : code)
	})

.

• cross-env version: 5.1.5
• node version: 10.1.0 (happens on earlier versions as well though)
• npm (or yarn) version: 6.0.1 (npm)

Relevant code or config

// exit code could be null when OS kills the process(out of memory, etc)
process.exit(code === null ? 1 : code)

Reproduction repository:
https://github.com/jnielson94/minimal-node-server

What you did:

Updated cross-env to 5.1.5 and started seeing errors when running npm scripts and killing them with cmd+c.

What happened:

The following log (really the whole file, but this is the important part with the error printout) illustrates my reproduction on as minimal a node process as I could get:

https://github.com/jnielson94/minimal-node-server/blob/914963cc14752eba9bc5b230182d53375f218a7f/cross-env.log#L23-L43

Basically, you get the super generic NPM error print out that results when a process exits with something other than 0, that tells you nothing about where the error actually happened (if there really was one), but without any logs above it telling you what the real issue was (it took some serious digging when we first realized the errors were consistent).

Problem description:
When node processes receive a SIGINT (and SIGTERM) node does some cleanup and re-raises the signal to actually exit the process. See docs. This seems to result in the re-raised signal terminating the process, but without an exit code, just a signal value. I wasn't able to dig into the node source enough to figure out why the exit event when handled this way doesn't have an exit code that cross-env receives in the listener, but I did find a possible solution.

Suggested solution:
I noticed in the original issue (#150) that they were having issues with SIGTERM still resulting in exit codes of 0. I propose that this section be updated to ensure SIGINT results in exit code 0:

cross-env/src/index.js

Lines 29 to 30 in eb37984

	// exit code could be null when OS kills the process(out of memory, etc)
	process.exit(code === null ? 1 : code)

.
The docs mention that the exit listener receives a second parameter - signal - with this statement regarding the two params: "If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null."

In theory, it is a pretty simple addition to the listener to ensure that a SIGINT results in an exit code of 0, but I hesitated to just open a PR that did that, as I wasn't sure how complex you were wanting the event handler to be in this case.

Something like this should work for the exit listener:

proc.on("exit", (code, signal) => {
  let crossEnvExitCode = code;
  // exit code could be null when OS kills the process(out of memory, etc) or due to node handling it
  // but if the signal is SIGINT the user exited the process so we want exit code 0
  if (crossEnvExitCode === null) {
    crossEnvExitCode = signal === "SIGINT" ? 0 : 1;
  }
  process.exit(crossEnvExitCode);
});

Sorry for not following the issue template exactly... I re-arranged things to try and make it read clearer.

[kentcdodds]

That looks like a good solution to me. PRs accepted!

[kentcdodds]

Thank you very much for your thorough work!

[jnielson94]

@kentcdodds I've run into an issue where the precommit hook is failing to find all-contributors-cli/cli module. I tried updating kcd-scripts (since it looks like a more recent version would have it fixed), but updating kcd-scripts past 0.5.0 results in the tests failing for cross-env (looks like something with mock behaviors changed)? Should I just disable the precommit hooks or do you know off-hand what might fix the tests?