Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

refactor: replace yarn with npm #1405

Merged
merged 12 commits into from
Oct 22, 2021
Merged

refactor: replace yarn with npm #1405

merged 12 commits into from
Oct 22, 2021

Conversation

zhaoyongjie
Copy link
Contributor

@zhaoyongjie zhaoyongjie commented Oct 13, 2021

💔 Breaking Changes

  • replace yarn with npm and remove dependency on yarn
  • bump eslint from 7.17.0 to 7.32.0
  • bump prettier from 2.2.1 to 2.4.1
  • remove @storybook/addon-info in superset-ui-demo as it isn't available on Storybook 6 and wasn't being used
  • remove dependency on @superset-ui/legacy-plugin-chart-word-cloud in superset-ui-demo (has been superseded by @superset-ui/plugin-chart-word-cloud)
  • freeze@encodable/color to 1.1.1 in superset-ui-demo

If you want to test this PR in local, please run commands in terminal

rm -rf ./{packages,plugins}/*/node_modules ./node_modules
rm -rf ./{packages,plugins}/*/yarn.lock ./yarn.lock
rm -rf ./{packages,plugins}/*/esm ./{packages,plugins}/*/lib
rm -rf ./{packages,plugins}/*/yarn-error.log ./yarn.lock
rm -rf ./{packages,plugins}/*/tsconfig.tsbuildinfo ./tsconfig.tsbuildinfo

refer to: [SIP-14] remove dependency on "Yarn" package in favor of npm

@vercel
Copy link

vercel bot commented Oct 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/9iLxXjvQzyhfEJ4CYVutvUAPezGT
✅ Preview: https://superset-ui-git-fork-zhaoyongjie-yarntonpm-superset.vercel.app

@zhaoyongjie zhaoyongjie changed the title [WIP]refactor: replace yarn with npm in superset-ui [WIP]refactor: replace yarn with npm Oct 13, 2021
@zhaoyongjie zhaoyongjie changed the title [WIP]refactor: replace yarn with npm refactor: replace yarn with npm Oct 14, 2021
@zhaoyongjie zhaoyongjie marked this pull request as ready for review October 14, 2021 09:57
@zhaoyongjie zhaoyongjie requested a review from a team as a code owner October 14, 2021 09:57
@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #1405 (aa5bc62) into master (746d399) will increase coverage by 0.01%.
The diff coverage is 32.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
+ Coverage   30.41%   30.43%   +0.01%     
==========================================
  Files         497      497              
  Lines        9996    10000       +4     
  Branches     1687     1689       +2     
==========================================
+ Hits         3040     3043       +3     
- Misses       6710     6711       +1     
  Partials      246      246              
Impacted Files Coverage Δ
...ols/src/components/ControlForm/ControlFormItem.tsx 0.00% <0.00%> (ø)
packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...s/superset-ui-core/src/chart/models/ChartPlugin.ts 100.00% <ø> (ø)
...erset-ui-core/src/models/RegistryWithDefaultKey.ts 100.00% <ø> (ø)
...kages/superset-ui-core/src/query/api/v1/makeApi.ts 100.00% <ø> (ø)
...ugins/legacy-plugin-chart-country-map/src/index.js 0.00% <0.00%> (ø)
plugins/legacy-plugin-chart-heatmap/src/index.js 0.00% <0.00%> (ø)
plugins/legacy-plugin-chart-map-box/src/index.js 0.00% <0.00%> (ø)
...gacy-plugin-chart-paired-t-test/src/TTestTable.jsx 0.00% <0.00%> (ø)
...n-chart-parallel-coordinates/src/transformProps.js 0.00% <0.00%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 746d399...aa5bc62. Read the comment docs.

@amitmiran137
Copy link
Contributor

Why are we replacing?
How is it align with monorepo project?

@michael-s-molina
Copy link
Contributor

@zhaoyongjie Can you add a reference to SIP-14 in the PR's description?

@zhaoyongjie
Copy link
Contributor Author

@zhaoyongjie Can you add a reference to SIP-14 in the PR's description?

Sure. Thanks.

@villebro
Copy link
Contributor

Why are we replacing? How is it align with monorepo project?

@amitmiran137 We're using npm on the main repo (see SIP-14: apache/superset#6217), and to simplify the toolchain, it makes sense to fully migrate to npm now. Yarn has also proven to be very bad at handling dependency resolution, due to which we're, among others, on incompatible versions of React for the current version of AntD (4.9.4 requires 17.x, we're on >=16.13).

@villebro
Copy link
Contributor

interim report: we are going to bump Storybook to 6.x (same version as on apache/superset) and then rebase this PR after that to make sure we'll be able to merge the superset-ui storybook with the storybook on the main repo.

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM

@eschutho
Copy link
Contributor

FYI the vercel storybook preview shows a broken chart for the legacy calendar. Not sure if it's related.
Legacy_Chart_Plugins___legacy-plugin-chart-calendar_-_Basic_⋅_Storybook

Copy link
Contributor

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

I left a few comments where npm ci would be better than npm install (mostly suggestions). But the release workflow is prob the most important one as npm doesn't have a --frozen-lockfile flag on install, it's essentially npm ci.

@zhaoyongjie
Copy link
Contributor Author

FYI the vercel storybook preview shows a broken chart for the legacy calendar. Not sure if it's related. Legacy_Chart_Plugins___legacy-plugin-chart-calendar_-_Basic_⋅_Storybook

The issues of Storybook will open separated PR to fix.

@zhaoyongjie
Copy link
Contributor Author

I left a few comments where npm ci would be better than npm install (mostly suggestions). But the release workflow is prob the most important one as npm doesn't have a --frozen-lockfile flag on install, it's essentially npm ci.

Nice catch. I am going to update this PR.

Copy link
Contributor

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

Looks great!

@zhaoyongjie zhaoyongjie merged commit 6bf1c57 into apache-superset:master Oct 22, 2021
@villebro
Copy link
Contributor

🎉 !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants