-
Notifications
You must be signed in to change notification settings - Fork 135
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
Build the frontend by esbuild instead of webpack #1063
Conversation
- init by `npx esbuild-create-react-app ui-esbuild`
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Hope we can get this merged before the Spring festival! It would be a giant step. |
@@ -129,6 +132,9 @@ function Sider({ | |||
// NOTE: Don't remove above comment line, it is a placeholder for code generator | |||
debugSubMenu, | |||
] | |||
if (topSQLSupport) { | |||
menuItems.splice(2, 0, topSQLMenu) |
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.
Insert topSQL menu.
hi @YiniXu9506 , can you help have a look why the e2e test always fails, thanks! |
Sure! |
All CI jobs are passed, PTAL, @breeswish , thanks! |
fail_ci_if_error: true | ||
flags: e2e_test | ||
verbose: true | ||
# - name: Upload coverage to 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.
What's wrong with this block?
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.
/cc @YiniXu9506
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.
The introduced esbuild causes the plugin in .babelrc does not work. I am working on this.
@@ -19,6 +19,10 @@ | |||
display: flex; | |||
flex-direction: column; | |||
padding-bottom: 20px; | |||
|
|||
.ant-menu.ant-menu-inline-collapsed { | |||
width: @menu-collapsed-width; // 80px |
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.
Is this an ant-design bug? If so, maybe you'd better propose a minimal reproducible example and file an issue.
// return mySqlFormatter.format(sql || '') | ||
// } | ||
|
||
import { format } from 'sql-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.
What's the impact of not using a TiDB SQL formatter? I believe something may get wrong.
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.
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.
But as we are specially introducing a TiDB formatter, will there be any feature breaks when it is not used?
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 manual tests, it looks the same as before, but I'm not 100% confident, I think we should add some unit tests for this method, I will book a todo.
]) | ||
|
||
// fetch data when instance id / time window size update | ||
useEffect(() => { | ||
handleUpdateTopSQLData() | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Do we have some safer way to do this magic? I mean, something that will not be reported by the lint while simple enough. I'm not an expert of React hook. How does other React hook users deal with this kind of case? @shhdgit
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.
OK, this is not a good scenario for the use of side effects. According to this article: https://stackoverflow.com/questions/58866796/understanding-the-react-hooks-exhaustive-deps-lint-rule
In the similar case, I should call the function directly when the event is triggered. Oh my react.
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.
The rest LGTM. I think we can fix trivial issues after the PR is merged.
Co-authored-by: Yini Xu <[email protected]> Co-authored-by: Suhaha <[email protected]>
Co-authored-by: Yini Xu <[email protected]> Co-authored-by: Suhaha <[email protected]>
What did:
Details:
.scss
files when compiling fluent-uibulma/dist/bulma.css
Result:
Speedup the compilation time nearly by 9x. In a high-performance desktop machine (CPU: Intel Core i5-9400F @ 6x 4.1GHz, Memory: 64GB), the full build costs near 70s by webpack while only near 8s by esbuild, and only near 5s to build js and css.
In the dev mode, after changing js or CSS code, esbuild needs near 600ms to make an incremental build.
Full build time with webpack (1.13 min, aka 67.8s for
webpack:build
stage):Full build time with esbuild (5.26s for
esbuild:build
stage):In dev mode, esbuild cost near 600ms to make incremental build:
CI
In CI, the
Build UI
job cost time is reduced from 2m 8s to 19s.Demo
Kapture.2021-12-02.at.22.18.40.mp4
Kapture.2021-12-02.at.22.13.52.mp4
Test
Tab 1:
Tab 2:
Tab 3:
$ cd ui $ yarn $ yarn start
Feedback to upstream
issues or PR reported to upstream: