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(ui): code-split out large xterm dep #12158

Merged
merged 7 commits into from
May 24, 2024

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Nov 7, 2023

Partial fix for #12059

Motivation

After the past few code-splits (#12150, #12097, #12061) xterm was left as one of the largest deps in the main bundle

  • xterm is only used by argo-ui in its LogsViewer component, which is only used by Workflows' FullHeightLogsViewer component
  • shave off more than half a MB from the main bundle:
    • from xterm itself: ~550KB / ~380KB minified / 84KB gzipped from main bundle
    • from argo-ui: ~190KB / ~60KB minified / ~17KB gzipped from main bundle
      • part of this went into the xterm bundle though, so there is some overlap
    • main bundle itself is now only 3.26MB / 1.64MB minified / ~430KB gzipped

Modifications

  • lazy load LogsViewer within FullHeightLogsViewer in order to code-split it

    • add a webpackPrefetch magic comment as logs are very commonly used, at least in my experience
  • import all argo-ui components individually, via argo-ui/src/components/*, instead of together via argo-ui or argo-ui/src/index

    • this makes tree-shaking argo-ui actually possible. the overall argo-ui import as well as each component imports styles, which are side-effects that make tree-shaking not possible
    • this also removed some argo-ui components from the bundle that are not used by Workflows

Verification

UI seems to work as before. Log viewing works as before.

Screenshot:

Screenshot 2024-05-14 at 2 41 04 PM

Bundle analysis

Before:

Screenshot 2024-05-14 at 2 51 42 PM
Screenshot 2024-05-14 at 2 51 54 PM

After:

Screenshot 2024-05-14 at 2 48 41 PM
Screenshot 2024-05-14 at 2 49 32 PM
Screenshot 2024-05-14 at 2 49 15 PM

Notes to Reviewers

  1. The main change is to full-height-logs-viewer.tsx, the rest is just import changes

  2. I thought of doing all the tree-shaking within a shim import, e.g. ./argo-ui-shim.ts, but then anything that imports that would get everything that the shim imports (due to its CSS import side-effects). Importing components individually where they're used allows for more splitting, particularly when more pages are split per UI: Code Split larger and/or less-used paths for smaller initial bundle #12059

- after the past few code-splits, `xterm` was left as one of the largest deps in the main bundle
  - `xterm` is only used by `argo-ui` in its `LogsViewer` component, which is only used by Workflows' `FullHeightLogsViewer` component
    - so lazy load `LogsViewer` within `FullHeightLogsViewer` in order to code-split it
      - add a [`webpackPrefetch`](https://webpack.js.org/guides/code-splitting/#prefetchingpreloading-modules) magic comment as logs are _very_ commonly used, at least in my experience

- **NOTE**: this is currently **not working** as a code-split as `argo-ui` does not seem to be tree-shakeable
  - so even with this code-split, the component is still being imported from other imports
  - from a quick glance, the lack of tree-shaking may be due to [this SCSS import](https://github.com/argoproj/argo-ui/blob/5ff344ac9692c14dd108468bd3c020c3c75181cb/src/components/index.ts#L1) in `argo-ui`, which is a side-effect
    - a potential workaround may be to import all the components individually and then import the styles individually in our own code
      - started on this, did not finish, still draft

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- this should be pretty much all components except for the few context references

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the refactor-ui-code-split-xterm branch from 36586e4 to 9cb1645 Compare May 14, 2024 19:27
@agilgur5
Copy link
Member Author

Merged main for new PR checks per #13027

@agilgur5 agilgur5 requested a review from tczhao May 22, 2024 19:27
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

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

LGTM. It was a good read, I learned quite a bit.

@agilgur5 agilgur5 merged commit 4d8f972 into argoproj:main May 24, 2024
16 checks passed
@agilgur5 agilgur5 deleted the refactor-ui-code-split-xterm branch May 24, 2024 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants