New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add Deno.exit.code
support
#23609
base: main
Are you sure you want to change the base?
Conversation
There is no test that ensures that the process actually exits with the given code. |
I'm not sure if the lint error has anything to do with this PR? It claims to be unhappy with |
@lukeed it's actually complaining about a missing copyright:
|
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 |
Should deno/ext/node/polyfills/process.ts Lines 403 to 405 in 1b27b58
|
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 |
@lukeed we discussed this API during the team meeting and the broad agreement was that |
All good, I'll defer to you all :) In order to get -- 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. |
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 |
Ok, I reverted to getter/setter pair & linked 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 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:
Basically, Node will parse non-nullish values to see if they're |
Thanks for the update @lukeed, sorry to hear your problems with rust-analyzer. I would suggest you add a config to it that uses
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 I'll make sure this lands in v1.44. |
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 Thanks! |
941b0f4
to
21930ef
Compare
Related to #23605
This pull request introduces the ability to set a would-be exit code for the Deno process without forcing an immediate exit, through the new
Deno.exit.code
API.Deno.exit.code
getter and setter: Adds support for setting and retrieving a would-be exit code viaDeno.exit.code
. This allows for asynchronous cleanup before process termination without immediately exiting.Deno.exit.code
validates that the provided value is a number, throwing a TypeError if not, to ensure that only valid exit codes are set.Deno.exit.code
utilizes theop_set_exit_code
operation to correctly register the intended exit code within Deno's internal mechanisms.cli/tests/unit/os_test.ts
verify the functionality of theDeno.exit.code
getter and setter, including type validation and the behavior that setting the exit code does not cause an immediate process exit.For more details, open the Copilot Workspace session.