Skip to content

Show a nice error message if user is running node <20 #6138

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

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

Conversation

JordanVerasamy
Copy link
Contributor

@JordanVerasamy JordanVerasamy commented Jul 21, 2025

WHY are these changes introduced?

https://github.com/Shopify/workflows-operations/issues/3367

WHAT is this pull request doing?

Show a nice error if user is running node version <20.

How to test your changes?

nvm install 18
nvm use 18
pnpm shopify store copy [...]

image.png

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@JordanVerasamy JordanVerasamy marked this pull request as ready for review July 21, 2025 19:40
@JordanVerasamy JordanVerasamy requested review from a team as code owners July 21, 2025 19:40
@JordanVerasamy JordanVerasamy force-pushed the require-node-at-least-20 branch from 575d9b1 to 609892b Compare July 21, 2025 19:53
@JordanVerasamy JordanVerasamy self-assigned this Jul 21, 2025
@JordanVerasamy JordanVerasamy force-pushed the require-node-at-least-20 branch from 609892b to faab565 Compare July 21, 2025 20:00
@JordanVerasamy JordanVerasamy force-pushed the require-node-at-least-20 branch from faab565 to 6c333a5 Compare July 21, 2025 21:20
Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
77.95% (+0.01% 🔼)
13012/16692
🟡 Branches
72.11% (+0.05% 🔼)
6343/8796
🟡 Functions
78.11% (+0.01% 🔼)
3372/4317
🟡 Lines
78.36% (+0.01% 🔼)
12322/15725
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / admin.ts
75% (-5.43% 🔻)
45.16% (+0.72% 🔼)
90.91%
76.47% (-5.75% 🔻)

Test suite run success

3046 tests passing in 1314 suites.

Report generated by 🧪jest coverage report action from 6c333a5

headline: 'Update to Node 20 or higher to run `shopify store copy`.',
body: [`The \`store copy\` command requires Node 20 or higher. Current Node version: ${nodeMajorVersion}.`],
})
process.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

try to avoid using process.exit and instead throw an error, that will be tracked.

Also, in cli-kit we have the exitIfOldNodeVersion already. It's currently checking for node <18, but maybe we can upgrade it to <20

Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Let's use the existing exitIfOldNodeVersion. For anything related to the system/environment/paths/filesystem, always check if there is a solution already in cli-kit (that's the goal, to have generic solutions for this kind of stuff)

@JordanVerasamy
Copy link
Contributor Author

Great feedback thanks!! Will adjust :)

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.

3 participants