-
Notifications
You must be signed in to change notification settings - Fork 640
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
fix(node): respect process.exitCode when exiting #1628
Conversation
node/process.ts
Outdated
super.emit("exit", process.exitCode); | ||
Deno.exit(process.exitCode || 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably wrong - if there are more "unload" listeners attached they will never be run. Opened the PR to get discussion going.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis any ideas how we tackle that except adding functionality equivalent to process.exitCode
directly into CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably wrong - if there are more "unload" listeners attached they will never be run. Opened the PR to get discussion going.
I have pushed a fix for this to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is blocked on denoland/deno#12888. This PR falsely makes it look like tests that fail actually succeed, because all deno test
isolates exit with code 0 if they import std/node/process.ts
anywhere now.
@bnoordhuis actually I was mistaken during the call, this PR is not yet ready. |
This should now work properly, however we should probably wait until denoland/deno#12938 is merged before merging this one. |
@bartlomieju, denoland/deno#12938 was cancelled. Is this PR still valid? |
Gently pinging @kt3k. |
@bartlomieju, this appears to no longer be an issue according to #1627 (comment) |
Thanks for checking @iuioiua, closing without a merge. |
Closes #1627