-
-
Notifications
You must be signed in to change notification settings - Fork 557
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): add descriptive error for unsupported node version #3894
Conversation
if (!semver.satisfies(process.versions.node, packageJSON.engines.node)) { | ||
console.error(`You are running Node.js version ${process.versions.node}, but Electron Forge requires Node.js ${packageJSON.engines.node}.`); | ||
process.exit(1); | ||
} | ||
|
||
import './util/terminate'; |
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.
Is there any particular reason for this to come before ./util/terminate
?
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.
I opted to have the version check before the other imports so that execution is immediately halted if an incompatible Node version is detected. The performance gain is probably negligible, but I thought it would better indicate the Node.js version as a critical prerequisite. However, I can also move all of the imports to the top of the file if you prefer the imports to be grouped.
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.
Yeah, I think I would prefer that. I took a quick look at ./util/terminate
and there isn't anything that seems like it would throw a Node version-related error and having all imports be at the top makes the code clearer.
Please take a look at these revisions and let me know what you think. I moved the imports for |
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.
However, I have the import for listr2 below the version check because it throws SyntaxError: Unexpected token '??=' when the Node.js version is below 16.
Haha I realize now that we actually had a copy of this code running through the Listr2 interface, which is useless if Listr2 is failing to load in the first place.
Can you clean up this bit of code as well as part of the PR? I think just removing the Node check from there would suffice.
forge/packages/api/cli/src/util/check-system.ts
Lines 134 to 138 in 5329eca
title: 'Checking node version', | |
task: async (_, task) => { | |
const nodeVersion = await checkNodeVersion(); | |
task.title = `Found node@${nodeVersion}`; | |
}, |
Good catch, let me know how this looks. |
Pushed up 2eacbaf as a nit but otherwise good to go. |
Summarize your changes:
Fixes #3554
This PR introduces an early Node.js version check in the
electron-forge.ts
CLI entry point. This prevents syntax errors caused by unsupported Node versions and provides a clear error message to the user.