node
node copied to clipboard
nodejs
process: add `code` validation in `process.exit()`
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 [email protected]
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?
IMHO, if there aren't any other node APIs that operate like this way, for consistency, semver-major seems convincing.
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 @tniessen Thanks for the review. I removed the range restriction, PTAL.
@RaisinTen Thanks for explaining the deprecation procedures and the search result. I will create a new PR.
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.
@aduh95 Thanks for the heads up. I updated process.md. Please let me know any other docs left.
On second thought, validating the process.exitCode seems to be a better approach for process.exit([code]).
- The following case is still possible since there is no validation on
process.exitCode.
process.exitCode = 'ERROR';
process.exit();
// Or
process.exitCode = 'ERROR';
- In the current version of
process.exit([code]),process.exitCodewill be set to thecodevalue if the parameter validation is passed. So,process.exit([code])can delegate the validation toprocess.exitCode.
- process.exitCode =
-
process.exitCode = undefined
- The doc says that the type of
process.exitCodeisinteger, but it should beinteger|undefined. Its default value isundefined.
- The doc says that the type of
Can you please take a look at https://github.com/nodejs/node/pull/43716/commits/d4ad2fe4aae7eced44b850c5baecfae9970049a6 again?
Thanks for reminding our deprecation process. :-) I'll do the follow-up after the next major release.
I'll do the follow-up after the next major release.
Would you be able to open a PR to add a runtime deprecation before the next major? v19.x semver cutoff is one month away, and if the runtime deprecation has not landed by then, this PR can't land until Node.js v21.x.. (assuming the runtime deprecation lands before Node.js 20.x semver cutoff)
Oh now I got it. I'll prepare a PR for a runtime deprecation so that it can be landed before the cutoff.
While rebasing the code I've squashed the previous commits because of many conflicts in several fixup commits. I will rebase this PR again once the release version for runtime deprecation is updated.
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3026/ CITGM(19.0.0): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3023/
The runtime deprecation has been versioned as v19.0.0. Rebased to resolve a conflict from the change.