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

refactor(monorepo): stage 1 #17427

Merged
merged 10 commits into from
Nov 17, 2021
Merged

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Nov 12, 2021

SUMMARY

Must merge apache-superset/superset-ui#1468 before

  1. large file checker skip .geojso
  2. upgrade eslint from 7.17.0 to 7.32.0
  3. upgrade prettier from 2.2.2 to 2.4.1
  4. format main repo
  5. edit lintrc
  6. add "prettier-plugin-packagejson": "^2.2.15" into dependency
  7. sort keys of packages.json by prettier-plugin-packagejson
  8. edit rat-excludes

To ensure that every PR passes the CI, the monorepo-related PR is split into multiple stages.

refactor(monorepo): stage 1 #17427 (this PR)
refactor(monorepo): relocate superset-ui #17445
refactor(monorepo): stage 2 #17437

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie force-pushed the monorepo_stage_1 branch 3 times, most recently from 0fdb894 to 2aa26d1 Compare November 15, 2021 03:32
@zhaoyongjie zhaoyongjie changed the title chore: monorepo stage 1 refactor: monorepo stage 1 Nov 15, 2021
@zhaoyongjie zhaoyongjie marked this pull request as draft November 15, 2021 05:39
@zhaoyongjie zhaoyongjie marked this pull request as ready for review November 15, 2021 14:26
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #17427 (2138679) into master (3ee9e11) will decrease coverage by 0.00%.
The diff coverage is 45.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17427      +/-   ##
==========================================
- Coverage   76.86%   76.85%   -0.01%     
==========================================
  Files        1042     1042              
  Lines       56254    56266      +12     
  Branches     7785     7782       -3     
==========================================
+ Hits        43242    43246       +4     
- Misses      12754    12764      +10     
+ Partials      258      256       -2     
Flag Coverage Δ
javascript 71.00% <45.37%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ontend/src/SqlLab/components/QuerySearch/index.tsx 76.53% <ø> (ø)
superset-frontend/src/chart/chartAction.js 50.47% <0.00%> (ø)
...rontend/src/components/AsyncEsmComponent/index.tsx 97.29% <ø> (ø)
...rset-frontend/src/components/ErrorMessage/types.ts 100.00% <ø> (ø)
.../ReportModal/HeaderReportActionsDropdown/index.tsx 24.32% <0.00%> (ø)
...ponents/Select/WindowedSelect/WindowedMenuList.tsx 10.71% <ø> (ø)
.../src/components/Select/WindowedSelect/windowed.tsx 88.23% <ø> (ø)
...frontend/src/dashboard/components/Header/index.jsx 68.85% <0.00%> (ø)
...tiveFilters/FiltersConfigModal/FilterTitlePane.tsx 92.00% <0.00%> (ø)
...ntend/src/dashboard/containers/DashboardHeader.jsx 100.00% <ø> (ø)
... and 86 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 3ee9e11...2138679. Read the comment docs.

update package.json

u package

pkg

pkg2
lint main repo

lint
lintrc 2

lintrc2

lintrc 3

lintrc
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM but a slightly blocking comment regarding newly disabled linting rules, curious to hear your thoughts.

Comment on lines +239 to +246
'jest/expect-expect': 0,
'jest/no-test-prefixes': 0,
'jest/valid-expect-in-promise': 0,
'jest/valid-expect': 0,
'jest/valid-title': 0,
'jest-dom/prefer-to-have-attribute': 0,
'jest-dom/prefer-to-have-text-content': 0,
'jest-dom/prefer-to-have-style': 0,
Copy link
Member

Choose a reason for hiding this comment

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

Are these ignores really needed? If this is only temporary we should probably add a todo or preferably to fix any violating tests. Especially jest/valid-expect-in-promise feels like a pretty bad violation, and these should probably be fixed to avoid flaky tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks ESlint change some default rule when I upgrade the ESlint. I will add a todo comment in these rules.
image

@zhaoyongjie zhaoyongjie changed the title refactor: monorepo stage 1 refactor(monorepo): stage 1 Nov 16, 2021
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,8 +31,8 @@ import { testWithId } from 'src/utils/testUtils';
import {
EchartsMixedTimeseriesChartPlugin,
EchartsTimeseriesChartPlugin,
} from '@superset-ui/plugin-chart-echarts/lib';
import { LineChartPlugin } from '@superset-ui/preset-chart-xy/lib';
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember why we imported from /lib here, but if this works, hooray!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think import from /lib was from IDE auto-generate code. import from /lib depend on package build, so we should avoid this pattern.

} from '@superset-ui/plugin-chart-echarts/lib';
import { LineChartPlugin } from '@superset-ui/preset-chart-xy/lib';
} from '@superset-ui/plugin-chart-echarts';
import { LineChartPlugin } from '@superset-ui/preset-chart-xy';
import TimeTableChartPlugin from '../../../../visualizations/TimeTable/TimeTableChartPlugin';
Copy link
Member

Choose a reason for hiding this comment

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

We should probably move this viz plugin to its own package as part of the bigger monorepo effort

return { data: list, totalCount: response.json.count };
});
},
() =>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm noting a lot of things not related to your PR, but this block is repeated a few times, and we could DRY it up later.

@@ -80,9 +80,9 @@ export default class AdhocMetric {

this.optionName =
adhocMetric.optionName ||
`metric_${Math.random()
`metric_${Math.random().toString(36).substring(2, 15)}_${Math.random()
Copy link
Member

Choose a reason for hiding this comment

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

This logic is also used here - maybe we could abstract it to a shared utility

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Noted a few little nitpicks for later cleanup, but here's the TODO items, just to list them here:
• Re-enabling linting rules
• Cleaning out unnecessary as unknowns
• Look for strings in need of translation
• Relocate TimeTableChartPlugin?

@rusackas
Copy link
Member

@geido or @michael-s-molina need to approve to merge, as codeowners of the Select component.

@zhaoyongjie zhaoyongjie merged commit 9070b6b into apache:master Nov 17, 2021
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* skip geojson in pre-commit

update prettier

* update package.json

update package.json

u package

pkg

pkg2

* lint main repo 2

lint main repo

lint

* lintrc

lintrc 2

lintrc2

lintrc 3

lintrc

* fix import

* refresh lock file

* fix break line make @ts-ignore invalid

* update rat-excludes

rat-excludes

update rat-excludes

* update eslintrc.js

* lint lint lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants