-
Notifications
You must be signed in to change notification settings - Fork 0
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
[task-37] add check for and upgrade npm and node versions #50
Conversation
.github/workflows/build.yml
Outdated
@@ -9,7 +9,7 @@ jobs: | |||
- name: Use Node.js ${{ matrix.node-version }} | |||
uses: actions/setup-node@v3 | |||
with: | |||
node-version: 17.x | |||
node-version: 18.x |
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.
should we just say 18.16.1 since we're using a specific version everywhere else?
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'm wondering if we should say 18.x everywhere else instead since the long term support version can update occasionally -- my list from nvm ls
lists lts/hydrogen
as 18.17.0
further down in the PR has >=18.16.1
listed in the engine properties so we should be consistent whether we want to be strictly on one version of node 18 or let it be 18.x
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.
If we say 18.x, won't it just go with the most recent which can conflict with current dependencies? Unless the fixed dependency is deprecated, I think we should pin the dependency to a specific version. If it gets deprecated, we can then update.
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'm not tied to any particular version of node 18, but yes, I agree, we may as well pin the version entirely. I'll update it to 18.17.0.
86366c0
to
abc97f8
Compare
Description
nvm
andnodenv
.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Feel free to include any video clips, or links if relevant for the testing done in this PR.
Ran and used app locally!