-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update minimum node version to 20 #443
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
Conversation
@@ -10,7 +10,7 @@ on: | |||
|
|||
env: | |||
WASP_TELEMETRY_DISABLE: 1 | |||
WASP_VERSION: 0.16.4 |
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.
This one is extra, but we might as well update?
@@ -29,6 +29,6 @@ | |||
"prisma": "5.19.1" | |||
}, | |||
"devDependencies": { | |||
"@types/node": "^18.0.0" |
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.
Not related to template or wasp,
we could actually have this at v24 if we wanted to.
Edit: maybe best to leave it at 20 so it follows wasp's minimum req
- name: Setup Node.js | ||
uses: actions/setup-node@v3 | ||
with: | ||
node-version: '18' |
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.
We could have this one at lts/*
since its not related to template or wasp.
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 a reason not to change it now?
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.
Just wanted somebody else to agree with me on this.
- name: Setup Node.js | ||
uses: actions/setup-node@v3 | ||
with: | ||
node-version: '18' |
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.
Same as above. lts/*
possible.
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.
Looks good!
When doing changes like this, I recommend always explaining how you searched for the references in the PR description (e.g., what you grepped for).
For example, I ran rg -i 'node.*18'
and see some package-lock.json
files listed. Should we update those too?
- name: Setup Node.js | ||
uses: actions/setup-node@v3 | ||
with: | ||
node-version: '18' |
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 a reason not to change it now?
I've "engines": {
"node": ">=18"
} I'm then gonna bump blog to |
Good thinking! |
Fixes: #442