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(cli): simplify empty event loop handling #6402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jul 19, 2024

What's the problem this PR addresses?

The error added in #6399 should have used throw new Error() so that Node.js can handle the printing and provide a stack trace.

How did you fix it?

Use throw new Error() instead of printing an error ourselves.

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz changed the title refactor(cli): simplify empty event loop handling fix(cli): simplify empty event loop handling Jul 19, 2024
@@ -187,17 +187,12 @@ export async function runExit(argv: Array<string>, {cwd = ppath.cwd(), selfPath,
const cli = getBaseCli({cwd, pluginConfiguration});

function unexpectedTerminationHandler() {
Cli.defaultContext.stdout.write(`ERROR: Yarn is terminating due to an unexpected empty event loop.\nPlease report this issue at https://github.com/yarnpkg/berry/issues.`);
throw new Error(`Yarn is terminating due to an unexpected empty event loop.\nPlease report this issue at https://github.com/yarnpkg/berry/issues`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the stacktrace matter? Wouldn't the only useful information be the place the aborted async operation was started (which we can't get anyway)?

Copy link
Member Author

@merceyz merceyz Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user wants to locate the file, other than that it doesn't provide much, it's more to let Node.js do the error handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep the output clean; at least something like this, similar to what we do in the catch?

Suggested change
throw new Error(`Yarn is terminating due to an unexpected empty event loop.\nPlease report this issue at https://github.com/yarnpkg/berry/issues`);
Cli.defaultContext.stdout.write(cli.error(new UsageError(`Yarn is terminating due to an unexpected empty event loop.\nPlease report this issue at https://github.com/yarnpkg/berry/issues`)));
process.exitCode = 1;

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.

2 participants