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: rethrow error when script failed #6460

Closed
wants to merge 1 commit into from

Conversation

MichaelBitard
Copy link
Contributor

Fixes #6399

It rethrows the error after being catched and printed.

This allows the exit-handler:

let flushed = 0
const flush = [process.stderr, process.stdout]
const exit = () => ++flushed === flush.length && process.exit(exitCode)
flush.forEach((f) => f.write('', exit))
to catch the error and propagate the error

@MichaelBitard MichaelBitard requested a review from a team as a code owner May 17, 2023 08:50
@MichaelBitard MichaelBitard changed the title rethrow error when script failed fix: rethrow error when script failed May 17, 2023
@wraithgar
Copy link
Member

Not throwing was intentional here. Running scripts in workspace mode is not intended to end immediately following the first error. This is also only a check for if there is no script w/ that name, so that you can run npm run x -ws and any workspaces with an x script will run it, and any that are missing will be ok. It's essentially an implicit --if-present mode.

At the end if ALL scripts were missing an error throws

throw new Error(`Missing script: ${args[0]}`)

If we want to make the --if-present explicit we would have to do that in a semver major (npm 10 is coming up) and make this code check that config and throw if it's not true. That's a wholly separate thing though.

If you want to try your hand at the breaking change feel free to make a PR for that. As it is though this PR is not something we can accept.

@wraithgar wraithgar closed this May 17, 2023
@MichaelBitard
Copy link
Contributor Author

Thanks for the detailed explanations!

The main issue is : when one script fails, the npm command ends without an error code.
That breaks a lot of things, and I thought re-throwing the error was the correct way (and I was wrong :))

Do you know how we could fix this issue?

@wraithgar
Copy link
Member

It is not supposed to exit with an error code. The implied --if-present is why. It only errors if they are all missing.

@wraithgar
Copy link
Member

Or do you mean if the script is present but exits uncleanly? Is this happening in every node version?

@wraithgar
Copy link
Member

If it's only in node20 this is the bug #6399

@MichaelBitard
Copy link
Contributor Author

Yes it's only on node20, and this current PR was trying to fix #6399 :)

That's why I'm asking you if you have some insights on how to fix this

@wraithgar
Copy link
Member

Ah I'm happy to help. I'll ping you in 6399 cause it has nothing to do w/ run-script.

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