-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
refactor(monorepo): stage 1 #17427
Conversation
0fdb894
to
2aa26d1
Compare
2aa26d1
to
10320f7
Compare
10320f7
to
75d8127
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
update prettier
update package.json u package pkg pkg2
lint main repo lint
d74964c
to
604b884
Compare
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 but a slightly blocking comment regarding newly disabled linting rules, curious to hear your thoughts.
'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, |
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.
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.
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.
rat-excludes update rat-excludes
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
superset-frontend/src/dashboard/components/PublishedStatus/PublishedStatus.test.tsx
Show resolved
Hide resolved
@@ -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'; |
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 can't remember why we imported from /lib
here, but if this works, hooray!
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 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'; |
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.
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 }; | ||
}); | ||
}, | ||
() => |
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.
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() |
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.
This logic is also used here - maybe we could abstract it to a shared utility
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.
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 unknown
s
• Look for strings in need of translation
• Relocate TimeTableChartPlugin
?
@geido or @michael-s-molina need to approve to merge, as codeowners of the Select component. |
* 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
SUMMARY
Must merge apache-superset/superset-ui#1468 before.geojso
eslint
from 7.17.0 to 7.32.0prettier
from 2.2.2 to 2.4.1"prettier-plugin-packagejson": "^2.2.15"
into dependencypackages.json
byprettier-plugin-packagejson
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