Skip to content
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: exit codes in node v20 #6461

Merged
merged 1 commit into from
May 17, 2023
Merged

fix: exit codes in node v20 #6461

merged 1 commit into from
May 17, 2023

Conversation

MichaelBitard
Copy link
Contributor

Fixes #6399

Properly this time I hope :)

@MichaelBitard MichaelBitard requested a review from a team as a code owner May 17, 2023 15:05
@wraithgar
Copy link
Member

wraithgar commented May 17, 2023

This subtly changes the behavior of when npm shows the "A complete log of this run can be found". Because this PR sets the exitCode variable no matter what, instead of only if we had a specific error, it will now show up during an npm audit where vulnerabilities are found. This is not what we want.

showLogFileError = (hasLoadedNpm && npm.silent) || noLogMessage
    ? false
    : !!exitCode

I think we have to leave exitCode undefined until we're ready to send it to process.exit and only then do we override it with process.exitCode if the exitCode variable is still undefined.

@MichaelBitard
Copy link
Contributor Author

I added a noLogMessage to true when something goes wrong, we can suppose the error message has already been printed.

I think we have to leave exitCode undefined until we're ready to send it to process.exit and only then do we override it with process.exitCode if the exitCode variable is still undefined.

I'm not sure to understand what it means.

If something set process.exitCode to something other than 0, that means the script failed at some point, and we should exit our process with an exitCode different than 0 rigth?

@wraithgar
Copy link
Member

If something set process.exitCode to something other than 0, that means the script failed at some point, and we should exit our process with an exitCode different than 0 rigth?

Correct, but we can't set our exitCode variable yet because that's doing some heavy lifting later on to determine if we need to show the extra logging message. Typically if a command sets exitCode but doesn't throw, that message isn't shown. Think of when you type npm foo, which isn't a command. You get an exit code of 1 but you don't get a message saying where to view the log of the run. That extra message is reserved for when a command throws an error. It's a subtle difference in signals from commands.

So, we have to wait till we call process.exit to see if we want to use exitCode or process.exitCode

@MichaelBitard
Copy link
Contributor Author

MichaelBitard commented May 17, 2023 via email

@wraithgar
Copy link
Member

Yep, you got there a different way by altering the other variable accordingly. It works, and smoke tests still pass. We'll add node 20 to our CI soon and cover any other holes soon hopefully.

@wraithgar wraithgar changed the title fix: exit code on workspace properly propagated in node v20 fix: exit codes in node v20 May 17, 2023
@wraithgar wraithgar merged commit 6ce99a8 into npm:latest May 17, 2023
@github-actions github-actions bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] exit codes not being set properly in node v20
2 participants