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

chore: change min npm version #5966

Closed
wants to merge 2 commits into from
Closed

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Feb 10, 2023

Description

Follow up of #5926

Test Plan

  • Run make clean to remove build, node_modules etc
  • Run npm i or make get-deps to install npm modules
  • Run make live to check if it works
  • Run make build to check if module can be built
  • Check if CI pass

Commentary (optional)

Adjusted package-lock.json for npm v9. Lock file version is now v3.
npm v9 is compatible with our current node version ^v16.13. resource

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for determined-ui failed.

Name Link
🔨 Latest commit bb53b00
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/63ea830213208a0008d794e8

@keita-determined keita-determined marked this pull request as draft February 10, 2023 18:25
Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

Great idea to address this! I left some comments.

@@ -4,7 +4,7 @@
"private": true,
"engines": {
"node": ">=16.13 <17",
"npm": ">=8.0.0"
"npm": ">=9.4.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

question: is there a way to have multiple npm version ranges? Specifically something like:

"npm": ">=8.0.0 <9.0.0, >=9.4.2"

If we can do something like that, then we can skip the use-npm-v9 step and just use the current npm version that comes with 16.14.2 for now and wait for the next node/npm upgrade so we don't have run an extra step to just update npm.

Copy link
Contributor Author

@keita-determined keita-determined Feb 13, 2023

Choose a reason for hiding this comment

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

i think we can do something like "npm": ">=8.0.0 <= 9.4.2". not sure we can do "npm": ">=8.0.0 <9.0.0, >=9.4.2". let me check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc says we can do "npm": ">=8.0.0 <9.0.0 || >=9.4.2"

@@ -100,6 +100,10 @@ commands:
- run: git submodule sync
- run: git submodule update --init

use-npm-v9:
steps:
- run: npm i -g [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit question: would this be better?

- run: npm i -g npm@^9.4.2

I can't remember the exact syntax but the idea is to get whatever newer patch version is compared to 9.4.2 so we're not specifically pinned to 9.4.2.

Copy link
Contributor Author

@keita-determined keita-determined Feb 13, 2023

Choose a reason for hiding this comment

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

I believe different npm version would cause some issues, but i think its more stable to use a specific npm version.
but the issue here is that - run: npm i -g npm doesnt work on circleci due to the permission error. not sure how to resolve the issue

this issue is resolved by "npm": ">=8.0.0 <9.0.0 || >=9.4.2"

@keita-determined
Copy link
Contributor Author

close this due to some issues

@keita-determined keita-determined deleted the chore/change-min-npm branch February 13, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants