-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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: upgrade to npm 7 and superset-ui 0.17.9 #13100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13100 +/- ##
==========================================
- Coverage 53.06% 48.12% -4.94%
==========================================
Files 489 492 +3
Lines 17314 17335 +21
Branches 4482 4486 +4
==========================================
- Hits 9187 8343 -844
- Misses 8127 8992 +865
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM and tested on node v12.20.2 + npm 7.5.4. It seems the node:12
docker image is still on npm 6, as I'm getting this when running docker-compose
: "npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it!"
@villebro Let me update the Dockerfile in this PR, too. |
5077f6e
to
8d9e2ab
Compare
- [Capitalization guidelines](#capitalization-guidelines) | ||
- [Sentence case](#sentence-case) | ||
- [How to refer to UI elements](#how-to-refer-to-ui-elements) | ||
- [\*\*Exceptions to sentence case:](#exceptions-to-sentence-case) |
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.
Updates from Markdown formatter
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.
LGTM, minor comment about Node.js name
CONTRIBUTING.md
Outdated
@@ -464,7 +473,7 @@ Frontend assets (TypeScript, JavaScript, CSS, and images) must be compiled in or | |||
|
|||
##### nvm and node | |||
|
|||
First, be sure you are using recent versions of NodeJS and npm. We recommend using [nvm](https://github.com/nvm-sh/nvm) to manage your node environment: | |||
First, be sure you are using recent versions of node.js and npm. We recommend using [nvm](https://github.com/nvm-sh/nvm) to manage your node environment: |
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 did some quick googling, and it appears the commonly used name is Node.js with a capital N: https://en.wikipedia.org/wiki/Node.js
SUMMARY
Closes #13067
#13079 upgraded
package-lock.json
to npm 7 format (lockfileversion: 2
), which breaks the current workflow withnpm link
(although #13058 has downgraded it tolockfileversion: 1
again):npm install
in each package you linkednpm install
will fail because of peer dependency resolution. You should usenpm install --legacy-peer-deps
until we resolve the peer dependency conflicts (seems solvable by upgrading to React 17).Instead of reverting, let's see what else we need to do to make the Superset codebase work with npm 7.
This PR updates the following:
engines
to npm 7@superset-ui/legacy-plugin-chart-map-box
which was left out of recent upgrades for a while... This brings in a couple of new commits in Superset UI:apache-superset/superset-ui@v0.17.8...v0.17.9
which adds a "sort by metric" checkbox for charts with single metric.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
ADDITIONAL INFORMATION