process: remove process.exit(), process.exitCode coercion to integer

[daeyeon]

This is a follow-up of runtime deprecation #44711.

This removes the deprecation, DEP0164, and validates the exit code for
both process.exit([code]) and process.exitCode.

Refs: #43738
Refs: #44714
Refs: #44711

Previous commit description

This commit validates the `code` in `process.exit([code])`.

According to the API doc, the code argument must be an integer value.
However, the current implementation accepts various types of values and
internally converts them into integer values. Also, the actual meaningful
code range is 0 - 255, but it doesn't validate the range.
The range of the code is OS-dependent.

The following are the current behaviors in the Linux shell:

$ node -e "process.exit('')"; echo $?
0
$ node -e "process.exit('2')"; echo $?
2
$ node -e "process.exit('256')"; echo $?
0
$ node -e "process.exit(-1)"; echo $?
255
$ node -e "process.exit([])"; echo $?
0
$ node -e "process.exit({})"; echo $?
0
$ node -e "process.exit('ERROR')"; echo $?
0

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

[Linkgoron]

Should this be semver-major, as this could easily cause crashes to apparently working programs?
if Node already handles non-string values in a somewhat sane fashion, maybe the docs need to be changed instead?

[daeyeon]

IMHO, if there aren't any other node APIs that operate like this way, for consistency, semver-major seems convincing.

[aduh95]

That's very much semver major, process.exit(-1) is a common value to indicate an error (https://github.com/search?q=process.exit%28-1%29&type=code), I don't think it's worth breaking at this point.

[aduh95]

If we're doing this, it must first go through a full deprecation cycle.

[tniessen]

Type-checking that the argument is an integer seems ok, but I wouldn't artificially restrict the range.

[daeyeon]

@aduh95 @tniessen Thanks for the review. I removed the range restriction, PTAL.

[RaisinTen]

The code change PR that implements the runtime deprecation should come after the doc-only deprecation PR. Take a look at #42149 for an example.

Overall, I think this is a good change because a quick search for process.exit(' in https://cs.github.com/?q=process.exit(%27%20language%3AJavaScript&scopeName=All%20repos&scope= tells tells me that there are a lot of invalid uses of this function.

Some assume that the string passed in would get logged somewhere, which is not right:
https://github.com/NikolaiT/uncaptcha3/blob/85d3f2f247bc2ae3a76b54e3d413a6738624bfc2/getCaptchaDownloadURL.js#L20

process.exit('Could not get ws URL');

And there are a lot of instances that seem to use process.exit() like process.kill() by passing in a signal name:
https://github.com/browserstack/browserstack-runner/blob/001204cce230cc1fb88c9159d6a11e481ab9b26b/lib/local.js#L29

process.exit('SIGINT');

[daeyeon]

@RaisinTen Thanks for explaining the deprecation procedures and the search result. I will create a new PR.

[BridgeAR]

I think strings that clearly represent an integer should continue to be supported as well as it would otherwise unnecessarly disrupt many users. The following should pretty much be accepted: input == null || Number.isInteger(input) || typeof input === 'string' && String(Number(input)) === input. That way weird strings would be rejected while integers get through.

[daeyeon]

@BridgeAR Applied your view. Thanks!

[aduh95]

Code LGTM. We should also update the docs.

[daeyeon]

@aduh95 Thanks for the heads up. I updated process.md. Please let me know any other docs left.

[daeyeon]

On second thought, validating the process.exitCode seems to be a better approach for process.exit([code]).

1. The following case is still possible since there is no validation on process.exitCode.

process.exitCode = 'ERROR';
process.exit();

// Or 

process.exitCode = 'ERROR';

2. In the current version of process.exit([code]), process.exitCode will be set to the code value if the parameter validation is passed. So, process.exit([code]) can delegate the validation to process.exitCode.

In a quick search for `process.exitCode` below, I can't find invalid uses but it may need the doc-only deprecation also.

• process.exitCode =
• process.exitCode = undefined
  • The doc says that the type of process.exitCode is integer, but it should be integer|undefined. Its default value is undefined.

Can you please take a look at d4ad2fe again?

[daeyeon]

I left the conflicts on purpose for easy review.

[aduh95]

That seems reasonable, however the docs for process.exitCode would need to be updated:

node/doc/api/process.md

Lines 1809 to 1813 in e0ab8dd

	## `process.exitCode`
	<!-- YAML
	added: v0.11.8
	-->

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47340/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47341/

[nodejs-github-bot]

Landed in 2d0d997