refactor(monorepo): stage 1 by zhaoyongjie · Pull Request #17427 · apache/superset

zhaoyongjie · 2021-11-12T19:57:29Z

SUMMARY

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

large file checker skip .geojso
upgrade eslint from 7.17.0 to 7.32.0
upgrade prettier from 2.2.2 to 2.4.1
format main repo
edit lintrc
add "prettier-plugin-packagejson": "^2.2.15" into dependency
sort keys of packages.json by prettier-plugin-packagejson
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

codecov · 2021-11-15T15:22:04Z

Codecov Report

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

@@            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 prettier

update package.json u package pkg pkg2

lint main repo lint

lintrc 2 lintrc2 lintrc 3 lintrc

villebro

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

villebro · 2021-11-16T08:05:44Z

+        '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,


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.

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

rat-excludes update rat-excludes

villebro

LGTM

rusackas · 2021-11-17T07:05:40Z

  EchartsMixedTimeseriesChartPlugin,
  EchartsTimeseriesChartPlugin,
-} from '@superset-ui/plugin-chart-echarts/lib';
-import { LineChartPlugin } from '@superset-ui/preset-chart-xy/lib';


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

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

rusackas · 2021-11-17T07:06:54Z

-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';


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

rusackas · 2021-11-17T07:12:31Z

-        return { data: list, totalCount: response.json.count };
-      });
-    },
+    () =>


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.

rusackas · 2021-11-17T07:19:00Z

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


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

rusackas

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 · 2021-11-17T07:28:10Z

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

@ts-ignore

* 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

@ts-ignore

* 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

zhaoyongjie requested review from geido and michael-s-molina as code owners November 12, 2021 19:57

pull-request-size Bot added the size/XXL label Nov 12, 2021

zhaoyongjie force-pushed the monorepo_stage_1 branch 3 times, most recently from 0fdb894 to 2aa26d1 Compare November 15, 2021 03:32

zhaoyongjie requested a review from villebro November 15, 2021 03:55

zhaoyongjie changed the title ~~chore: monorepo stage 1~~ refactor: monorepo stage 1 Nov 15, 2021

zhaoyongjie marked this pull request as draft November 15, 2021 05:39

zhaoyongjie force-pushed the monorepo_stage_1 branch from 2aa26d1 to 10320f7 Compare November 15, 2021 11:09

zhaoyongjie mentioned this pull request Nov 15, 2021

refactor(monorepo): move superset-ui to superset(stage 2) #17437

Closed

zhaoyongjie force-pushed the monorepo_stage_1 branch from 10320f7 to 75d8127 Compare November 15, 2021 14:25

zhaoyongjie marked this pull request as ready for review November 15, 2021 14:26

rusackas reviewed Nov 15, 2021

View reviewed changes

Comment thread superset-frontend/.eslintrc.js Outdated

rusackas reviewed Nov 15, 2021

View reviewed changes

Comment thread superset-frontend/cypress-base/cypress/utils/parsePostForm.ts Outdated

zhaoyongjie added 7 commits November 16, 2021 13:02

skip geojson in pre-commit

87a06a9

update prettier

update package.json

c85ced8

update package.json u package pkg pkg2

lint main repo 2

3a72dd1

lint main repo lint

lintrc

38b51fd

lintrc 2 lintrc2 lintrc 3 lintrc

fix import

1ddd5d6

refresh lock file

addf007

fix break line make @ts-ignore invalid

604b884

zhaoyongjie force-pushed the monorepo_stage_1 branch from d74964c to 604b884 Compare November 16, 2021 05:02

villebro reviewed Nov 16, 2021

View reviewed changes

zhaoyongjie changed the title ~~refactor: monorepo stage 1~~ refactor(monorepo): stage 1 Nov 16, 2021

zhaoyongjie mentioned this pull request Nov 16, 2021

refactor(monorepo): relocate superset-ui #17445

Merged

9 tasks

zhaoyongjie added 2 commits November 16, 2021 17:11

update rat-excludes

74ac58c

rat-excludes update rat-excludes

update eslintrc.js

5dfb68e

villebro approved these changes Nov 16, 2021

View reviewed changes

lint lint lint

2138679

zhaoyongjie requested a review from rusackas November 16, 2021 12:21

ktmud approved these changes Nov 17, 2021

View reviewed changes

rusackas reviewed Nov 17, 2021

View reviewed changes

Comment thread superset-frontend/src/dashboard/components/PublishedStatus/PublishedStatus.test.tsx

rusackas reviewed Nov 17, 2021

View reviewed changes

rusackas approved these changes Nov 17, 2021

View reviewed changes

geido approved these changes Nov 17, 2021

View reviewed changes

zhaoyongjie merged commit 9070b6b into apache:master Nov 17, 2021

zhaoyongjie mentioned this pull request Nov 25, 2021

refactor(monorepo): move superset-ui to superset(stage 2) #17552

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 First shipped in 1.5.0 labels Mar 13, 2024

Conversation

zhaoyongjie commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

codecov Bot commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

villebro Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

zhaoyongjie Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rusackas Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

zhaoyongjie Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

rusackas Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

rusackas Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

rusackas Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

rusackas left a comment

Choose a reason for hiding this comment

Uh oh!

rusackas commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhaoyongjie commented Nov 12, 2021 •

edited

Loading

codecov Bot commented Nov 15, 2021 •

edited

Loading