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 gigantic Monaco Editor dep #12150

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Nov 5, 2023

Partial fix for #12059 -- this is the largest improvement by a wide margin, though still a little bit left to do
Partial fix for #11740

Motivation

  • shave off 7.72MB / 3MB minified / 762KB gzipped from main bundle
    • main bundle itself is now only 5.11MB / 2.81MB minified / 553KB gzipped
      • i.e. the entire main bundle is smaller than the Monaco bundle 😮 😬

Modifications

  • wrap react-monaco-editor into a lazy loaded component

    • with some tiny gimmicks to it per in-line comments
  • in ObjectEditor, asynchronously import languages as well in one of its effects

    • otherwise all of monaco-editor gets imported, per in-line comment
    • also rewrite that effect to use modern async/await instead of promise chains
      • which ends up a good bit simpler than what it would be with a promise chain with the new async import on top of the async fetch etc
      • and invert a conditional as well to reduce nesting / code complexity
  • also refactor ObjectEditor a bit to correctly use useRef instead of createRef

    • createRef is for legacy class-based components, while useRef is for hooks-based functional components
  • add webpackChunkName magic comment for clearer names for all of the dynamic imports

    • note that sub-chunks (like Chart.js, which is a chunk of reports) can't be directly named
      • output.chunkFilename is how it's typically configured, but you can't access the unresolved import name in that
        • [name] is equivalent to [id] in this case; I tried

Verification

Tested locally, editor seems to work fine in the UI:
Screenshot 2023-11-04 at 11 40 43 PM

Bundle verification

Full bundle, monaco-editor now split out:
Screenshot 2023-11-05 at 12 27 22 AM - split monaco

Size of remaining main bundle:
Screenshot 2023-11-05 at 12 28 05 AM - main wo monaco

- shave off 7.72MB / 3MB minifed / 762KB gzipped from main bundle
  - main bundle itself is now only 5.11MB / 2.81MB minified / 553KB gzipped
    - i.e. the entire main bundle is smaller than the Monaco bundle 😮 😬

- wrap `react-monaco-editor` into a lazy loaded component
  - with some tiny gimmicks to it per in-line comments

- in `ObjectEditor`, asynchronously import `languages` as well in one of its effects
  - otherwise all of `monaco-editor` gets imported, per in-line comment
  - also rewrite that effect to use modern async/await instead of promise chains
    - which ends up a good bit simpler than what it would be with a promise chain with the new async import on top of the async fetch etc
    - and invert a conditional as well to reduce nesting / code complexity

- also refactor `ObjectEditor` a bit to correctly use `useRef` instead of `createRef`
  - the former is for legacy class-based components, the latter is for hooks-based functional components

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- my editor defaults to 2 space, oops
  - some of the code I copy+pasted (from previous code I wrote) was 4 space though, so ended up with mixed indentation 😕
  - weirdly didn't pop up when I ran `yarn lint` locally 🤔, possibly b/c I hadn't staged it yet?

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- to make the bundle names a bit clearer
  - although pieces that got code split out of the dynamic imported bundles (e.g. `Chart.js` for instance) still have randomized names
    - that might require some fancy function to produce a cleaner `output.chunkName`, not sure

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added area/ui area/build Build or GithubAction/CI issues labels Nov 5, 2023
- my local lint was erroring on an unstashed file (due to a TS version issue), so that's why it failed to reformat everything

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@terrytangyuan terrytangyuan merged commit 453f85f into argoproj:master Nov 20, 2023
18 checks passed
@agilgur5 agilgur5 deleted the refactor-ui-code-split-monaco branch November 20, 2023 20:11
shuangkun pushed a commit to shuangkun/argo-workflows that referenced this pull request Nov 21, 2023
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
terrytangyuan pushed a commit that referenced this pull request Nov 27, 2023
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
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.

3 participants