-
Notifications
You must be signed in to change notification settings - Fork 360
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: update node version to active LTS (DET-7046) #3932
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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 added some comments but overall LGTM.
If we go with the legacy npm config option, it'd be good to see when we can take the added option out.
From a search I did previously it looked like we should be able to get rid of it once react-scripts
& CRA release new versions and craco
adopts them
https://github.com/gsoft-inc/craco/releases/tag/v7.0.0-alpha.0
webui/react/package.json
Outdated
"node": "^14.16.0", | ||
"npm": "<7.0.0" | ||
"node": "^16.14.2", | ||
"npm": "^8.7.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.
nit: match CONTRIBUTING.md ?
@@ -0,0 +1 @@ | |||
legacy-peer-deps=true |
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.
It'd be ideal to see if we can upgrade our packages so we don't need this but it sounds like this is a good step forward and strictly a win.
https://github.blog/2021-02-02-npm-7-is-now-generally-available/#peer-dependencies
You have the option to retry with --force to bypass the conflict or --legacy-peer-deps command to ignore peer dependencies entirely (this behavior is similar to versions 4-6).
could you also please check that storybooks builds (dev & production) are not affected? |
This reverts commit 8616ebb.
i think we need |
Description
Updating node from v12.14 to v16.14.2
Also updating npm version to latest, v8.7.
Test Plan
should not require additional testing as long as everything builds and regression tests pass
Commentary (optional)
Updating from npm v8.5 to v8.7 requires one of two solutions:
legacy-peer-deps=true
([BUG] non-previosly seen peer-dependency errors popping up in 8.6.0 npm/cli#4664)OR
craco-sass-resources-loader
devDependecy with a fork,@jonny1994/craco-sass-resources-loader
Checklist
docs/release-notes/
.See Release Note for details.