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

feat: create exit handler class #7442

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 30, 2024

This PR refactors exit-handler.js to be a class so that it can more easily track its internal state. It uses this state to now fully distinguish between 3 states: npm never being set, npm not loaded, and exit handler never called. There are some new error messages shown via console.error if we know we are in an unexpected state.

This also continues the refactoring started in #7415 to separate concerns between npm and CLI. Identifying the error message and logging it have been move to npm but catching that error and setting the process.exitCode are still handled in exit-handler.js and cli/entry.js.

It also moves command.cmdExec to npm since it never called from within any command instance. This lets npm only ever call command.exec or command.workspaceExec now.

@lukekarrys lukekarrys force-pushed the lk/exit-handler-class-2 branch 10 times, most recently from 8df95d2 to 9770c40 Compare April 30, 2024 04:18
@lukekarrys lukekarrys marked this pull request as ready for review May 2, 2024 16:39
@lukekarrys lukekarrys requested a review from a team as a code owner May 2, 2024 16:39
@@ -66,7 +66,6 @@ npm error
npm error aliases: clean-install, ic, install-clean, isntall-clean
npm error
npm error Run "npm help ci" for more info

Copy link
Member

Choose a reason for hiding this comment

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

this newline is present in most other situations where the complete log message is displayed.

Copy link
Member

Choose a reason for hiding this comment

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

Testing locally this newline is now gone altogether in those other situations. Consistency is the thing here so we're good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, glad you noticed. I couldn't find the true historical reason for this, but in refactoring I'm not confident we always display at least something to the user in all cases (even weird ones like npm is never set and then we error). But since this is truly just a display oriented newline, I wanted to remove it.

@lukekarrys lukekarrys merged commit 1e375c1 into latest May 6, 2024
34 checks passed
@lukekarrys lukekarrys deleted the lk/exit-handler-class-2 branch May 6, 2024 16:38
@github-actions github-actions bot mentioned this pull request May 6, 2024
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