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

UI: Code Split larger and/or less-used paths for smaller initial bundle #12059

Open
8 of 19 tasks
agilgur5 opened this issue Oct 21, 2023 · 6 comments
Open
8 of 19 tasks
Assignees
Labels
area/ui solution/suggested A solution to the bug has been suggested. Someone needs to implement it. type/feature Feature request

Comments

@agilgur5
Copy link
Contributor

agilgur5 commented Oct 21, 2023

Summary

Instead of having all the UI pages in one bundle, we can code split out certain pages that are either quite large (e.g. have a lot of code or deps) or less-used.

Use Cases

Implementation Details

Nowadays, React supports loading components async with the built-in lazy function, which you normally pass a dynamic import into. Webpack will automatically code-split dynamic imports.

Two components that I think might be ripe for code-splitting are:

Also, some pages could be good to code-split by default

Checklist

EDIT: This was added after a lot of analysis and work below to keep track of things, was not part of the original issue.

In order of most impact to the bundle and most splittable (some are used in many places and so are harder to split):

  1. code split deps

    1. monaco-editor (et al): refactor(ui): code-split gigantic Monaco Editor dep #12150
    2. moment-timezone: refactor(ui): replace moment-timezone with native Intl #12097
    3. swagger-ui-react (et al): build(ui): code-split ApiDocs and Reports components #12061
    4. xterm: refactor(ui): code-split out large xterm dep #12158
    5. moment: refactor(deps): remove moment dep and usage #12611
    6. chart.js: build(ui): code-split ApiDocs and Reports components #12061
    7. react-markdown (+ remark-gfm + etc): fix(ui): code-split markdown title + desc, fix row linking, etc #12580
    8. react-datepicker (+ date-fns + react-popper + etc):
    9. cron-parser (+ luxon):
  2. code split pages

    1. ApiDocs: build(ui): code-split ApiDocs and Reports components #12061
    2. Reports: build(ui): code-split ApiDocs and Reports components #12061
    3. event-sources:
    4. sensors:
    5. event-flow:
    6. cron-workflows:
    7. workflow-templates:
    8. cluster-workflow-templates:

Trade-offs

Note that code-splitting is not without its trade-offs as more bundles can have downsides. In particular if those bundles are very small. In this case I think the pros outweigh the cons for certain pages (Swagger UI almost certainly), but may need some more testing for other pages.


Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

@agilgur5 agilgur5 added type/feature Feature request area/ui labels Oct 21, 2023
@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 21, 2023

Ran webpack-bundle-analyzer (which I had wanted to do for a while, figured now's a good time if I wrote this issue already) and here are the results:

Screenshot 2023-10-21 at 12 24 19 PM

Observations:

  1. monaco-editor is gigantic and takes up almost half of the bundle. I'm pretty sure the workers are from Monaco as well
    • I expected it to be large per above, but did not quite expect half of the bundle 😬
  2. swagger-ui-react is the 6th largest package (after monaco-editor, react-dom, moment-timezone, moment, and argo-ui).
    • Similarly expected per above. That is large, but also comparatively tiny compared to monaco-editor 😅
    • swagger-client is also listed separately
    • EDIT: it also has some other large deps that only it uses, such as immutable, remarkable, and dompurify (see below)
  3. chart.js is 7th
    • Mentioned above re: Reports page. That's the only place it's used
  4. moment, luxon, and date-fns are all in the bundle 🙃 😬
    • these all have similar functionality and are all fairly large (with moment taking up the 3rd and 4th largest spots)
      • they also seemed to not have been tree-shaken well? not sure if the transitive deps that use them are importing in a tree-shakeable fashion or using tree-shaking options in their bundlers
      • EDIT: cron-parser uses luxon, react-datepicker uses date-fns, and we use moment 🙃
        • EDIT2: chart.js uses moment too
    • moment is the only one we use directly. unfortunately though, argo-ui also uses it directly, so this is not necessarily the simplest to remove as a result
  5. rxjs is not as large in comparison, though it is still 9th
    • My prior assumption was that it was a good one to remove too (esp with how functional JS has become in the past decade). It would still be good to remove, but not as high on the priority list (also rxjs adds a bit of learning curve for contributors too, so there are other reasons for removing it)
  6. immutable and redux (and react-redux) are in the bundle from some deps 🙃
    • EDIT: swagger-ui-react uses redux although react-form uses it too
    • EDIT: swagger-ui-react also uses immutable, though sass uses it too -- but that's only a devDep (and also ripe for a more modern replacement)
  7. remarkable is in there too??
    • EDIT: this is actually only used by swagger-ui-react
  8. xterm is pretty large and is only used by argo-ui for the LogsViewer component.
    • That could potentially be replaced by a smaller component that does a lot less than xterm, since we're only using it for the LogsViewer
  9. The actual Workflows code is pretty tiny, as expected

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 21, 2023

Argo CD appears to do code-splitting in one place already -- for monaco-editor. That code looks like it is almost as old as Argo CD itself (4+ years old), so I'm surprised it never made it into Workflows 🤷

I believe both CD and Workflows only do very minimal code editing of YAML files, so we likely could use less featured editors as well (YAML is a lot simpler than a full language).

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 21, 2023

Tiny modification, in my above comment I accidentally analyzed a development bundle (as visible in react-dom.development.js), not a production bundle. A prod bundle can remove quite a bit of dev code, dead code, unused code, and minification can result in some changes too.

Here's an analysis of a production bundle:
Screenshot 2023-10-21 at 3 50 47 PM -- before prod bundle

Main differences are:

  • monaco-editor is a bit smaller of a percentage, but still gigantic
  • moment-timezone, swagger-ui-react, and xterm are now all larger percentages as the 2nd, 3rd, and 4th largest packages after monaco-editor
    • chart.js is now 6th, rxjs now 7th
  • react-dom's prod bundle is significantly smaller than the dev bundle
    • more or less expected, since React team had done a lot of work in optimizing that in the past and there are quite a lot of dev-mode-only functionalities

@agilgur5
Copy link
Contributor Author

agilgur5 commented Nov 5, 2023

Also thought I'd output some Webpack performance hints/warnings (after #12150):

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  json.worker.js (359 KiB)
  assets/jsonschema/schema.json (499 KiB)
  assets/fonts/fa-solid-900.ttf (385 KiB)
  assets/openapi-spec/swagger.json (672 KiB)
  main.0ade2a2f5ba2aa19fd60.js (2.81 MiB)
  225.38a4610abeb432479adf.js (826 KiB)
  322.8bcbc047db9e1371c28e.js (3.07 MiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (2.81 MiB)
      main.0ade2a2f5ba2aa19fd60.js

Webpack's perf hints suggest going down to ~244KB minified.
I'm honestly not sure if that would be possible to hit, but that would require doing all the dependency code-splitting / pruning as per above, as well as probably splitting up each route in the UI into its own bundle. And even then, I'm still not sure it'd be small enough.

In any case, I'm still focusing on the large deps first and foremost, but thought I'd note that down for reference

@Adrien-D
Copy link
Member

Is there anything that could prevent us from upgrade react-router & react-router-dom to v6 to be able to use lazy straight from the router ? That could be a big win for bundle size

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 26, 2024

That would only solve the pages portion of this issue, but not the deps portion (some large deps are used in multiple pages/routes). Ultimately, that's also just short-hand, so just simplifies that part of the issue

But yes, there's a chicken-and-egg problem with upgrading react-router: argo-ui uses it and other Argo projects are not necessarily ready/compatible with an upgrade to a newer react-router version. See argoproj/argo-ui#481 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui solution/suggested A solution to the bug has been suggested. Someone needs to implement it. type/feature Feature request
Projects
Status: No status
Development

No branches or pull requests

2 participants