feat: Add Deno.exitCode API

[lukeed]

This commits adds the ability to set a would-be exit code
for the Deno process without forcing an immediate exit,
through the new Deno.exitCode API.

• Implements Deno.exitCode getter and setter: Adds support for setting
  and retrieving a would-be exit code via Deno.exitCode.
  This allows for asynchronous cleanup before process termination
  without immediately exiting.
• Ensures type safety: The setter for Deno.exitCode validates that
  the provided value is a number, throwing a TypeError if not, to ensure that
  only valid exit codes are set.

Closes to #23605

[lucacasonato]

There is no test that ensures that the process actually exits with the given code.

[lukeed]

I'm not sure if the lint error has anything to do with this PR? It claims to be unhappy with 01_core.js which wasn't modified in this PR

[bartlomieju]

@lukeed it's actually complaining about a missing copyright:

Copyright header is missing: /home/runner/work/deno/deno/cli/tests/unit/os_test.ts (incorrect line is '// This file contains unit tests for the Deno.exit.code API')

[bartlomieju]

Also I think it would be really good to add one integration test that checks that the set exit code is actually used when the process exits - could you look into that by following one of the existing tests in tests/specs/run/?

[Seally]

Should Deno.exit.code always return the same value as process.exitCode? process.exitCode also stores its own exit code as a JS-side variable, so as far as I can tell at least one of them will be wrong once either setter is called.

deno/ext/node/polyfills/process.ts

Lines 403 to 405 in 1b27b58

	get exitCode() {
	return globalProcessExitCode;
	}

[bartlomieju]

Should Deno.exit.code always return the same value as process.exitCode? process.exitCode also stores its own exit code as a JS-side variable, so as far as I can tell at least one of them will be wrong once either setter is called.

deno/ext/node/polyfills/process.ts

Lines 403 to 405 in 1b27b58

	get exitCode() {
	return globalProcessExitCode;
	}

Thanks for pointing this out, they should share the same value actually.

@lukeed let me know if you need any help with this PR, it might be more involved since we should share the same code between Deno.exit.code and Node compat layer. I'll be happy to help.

[bartlomieju]

@lukeed we discussed this API during the team meeting and the broad agreement was that Deno.exitCode would be a better choice. Is there a particular reason why you abandoned the previous approach?

[lukeed]

All good, I'll defer to you all :)

In order to get Deno.exitCode to work, it has to be a getter and a setter. However, the only way to do that is by converting the entirety of 30_os.js to use export default so that the top-level getter/setter pair can be defined.

-- export {
++ const os = {
  env,
  execPath,
  exit,
++ get exitCode() { ... },
++ set exitCode(value) { ... },
  gid,
  hostname,
  loadavg,
  networkInterfaces,
  osRelease,
  osUptime,
  setExitHandler,
  systemMemoryInfo,
  uid,
};

++ export default os;

I abandoned that approach because (i assumed) it would mess up the NS builder since everything else is using named exports.

[bartlomieju]

Got it, thanks for the explainer. I'll take a deeper look at it this week, maybe we can figure something out :)

EDIT: Actually taking a quick look - I think it would work if we exported getExitCode and setExitCode from 30_os.js and then in 90_deno_ns.js we added get exitCode() and set exitCode() to denoNs object as you describe above. It appears we'll need to add op_get_exit_code to make sure both "Deno" native and "Node" native APIs see the same code properly, but that seems easy enough. WDYT @lukeed?

[lukeed]

Ok, I reverted to getter/setter pair & linked process.exit/Code too.

TBH, between rust-analyzer, ts server, the 106k files w/ system scanning, my computer literally grinds to a halt :/ I wait about 5s per keystroke lolsob. So if anything else is needed, someone else will have to carry the torch 🙇

Aside from not knowing where to put Node process tests, the only issue I can see is this:

function demo() {
  process.exitCode = '123'; 
  process.exitCode; // -> "123" (unfortunately)
  // then, for some reason
  Deno.exitCode = 1;
  process.exitCode; //-> "123" (does not see Deno update)
  process.exitCode = "123";
 //-> process exits w/ 123
}

It gets weird if you're using both Deno & Node APIs at the same time. But choosing one camp always has Deno exiting correctly.

The only ways I can see this being fixed are:

1. the Deno.exitCode setter calls into process.exitCode
   but this is bad because it perma-links the Node polyfill runtime w/ Deno namespace

2. make the Deno NS behave like Node & hold/return the raw value until it actually exits.
   That would mean not calling op_set_exit_code until Deno.exit() and Deno.exitCode would have to be able to return string | number | null | undefined like Node does. (yuck)

Basically, Node will parse non-nullish values to see if they're NaN and, if so, throw a TypeError. But then it always returns the raw value.

[bartlomieju]

Thanks for the update @lukeed, sorry to hear your problems with rust-analyzer. I would suggest you add a config to it that uses --feature __runtime_js_sources - this should save some time on each change as it would have to do a lot less work. I can take over the PR and finish it.

Aside from not knowing where to put Node process tests, the only issue I can see is this:

It gets weird if you're using both Deno & Node APIs at the same time. But choosing one camp always has Deno exiting correctly.

The only ways I can see this being fixed are:

the Deno.exitCode setter calls into process.exitCode
but this is bad because it perma-links the Node polyfill runtime w/ Deno namespace

make the Deno NS behave like Node & hold/return the raw value until it actually exits.
That would mean not calling op_set_exit_code until Deno.exit() and Deno.exitCode would have to be able to return string | number | null | undefined like Node does. (yuck)

Thanks for pointing this out, I think I know how to handle that correctly. Point 1 is not really a concern since that's already the case 🫠 For point 2 I think for Deno API we can try to coerce the value to a number and also throw a TypeError if it can't be parsed. So Deno.exitCode: number | undefined.

I'll make sure this lands in v1.44.

[lukeed]

we can try to coerce the value to a number and also throw a TypeError if it can't be parsed

This already happens :) I was pointing out that if you wanted to have perfect synchronization with Node, you'd have to change Deno side to allow for more than number|undefined to be the return type.

Thanks!

[dsherret]

LGTM!

[dsherret]

Might be a little confusing, but seems ok.

[lukeed]

Thank you @bartlomieju!! :D