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

build(ui): code-split ApiDocs and Reports components #12061

Merged

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Oct 21, 2023

Partial fix for #12059

  • still more code splitting that can be done

Motivation

  • swagger-ui-react was one of the biggest dependencies and only used on the infrequently used /apiDocs page
    • and it was the sole importer of large dependencies like dompurify, immutable, redux, etc
    • shave off ~2MB / 826KB minified / 250KB gzipped from the main bundle
  • charts.js was also one of the bigger deps and similarly only used on the infrequently used /reports page
    • shave off ~525KB / 190KB minified / 60KB gzipped from the main bundle

Modifications

Verification

See screenshots bundle analysis after this code splitting was added below:
Screenshot 2023-10-21 at 3 14 45 PM -- after code-split apiDocs and Reports
Screenshot 2023-10-21 at 3 10 22 PM -- after code-split apiDocs and Reports

Tested locally and worked without issues.
I barely even noticed the lazy load as it took less than a second for it to appear, and I was specifically paying attention for it. Results may vary ofc, esp since locally the network is ofc much faster, but per #12059 I still think this is well worth the tiny trade-off given how rarely used these pages are.

Future Work

There's still more to code-split per #12059

  • these two were particularly quick as they were their own pages and isolated to a single module
  • monaco-editor would be good to do next; I didn't do it as part of this because it requires more granular changes to two different components/modules -- need to take some more time with that

- `swagger-ui-react` was one of the biggest dependencies and only used on the infrequently used `/apiDocs` page
  - and it was the sole importer of large dependencies like `dompurify`, `immutable`, `redux`, etc
  - shave off ~2MB / 826KB minified / 250KB gzipped from the main bundle
- `charts.js` was also one of the bigger deps and similarly only used on the infrequently used `/reports` page
  - shave off ~525KB / 190KB minified / 60KB gzipped from the main bundle

- use `React.lazy` to lazy load them with dynamic imports
  - and `React.Suspense` to add a fallback etc
  - see https://react.dev/reference/react/lazy and https://webpack.js.org/guides/code-splitting/#dynamic-imports
    - also https://v5.reactrouter.com/web/guides/code-splitting, though it's a bit dated now (`lazy` and `Suspense` are built-in now, no need to use `loadable-components`)

- make some small upgrades to support dynamic importing
  - per comments, `ES2020`+ `module` format is required for dynamic imports (otherwise they get transpiled to regular, synchronous `require`s)
    - note that Argo CD [already uses this](https://github.com/argoproj/argo-cd/blob/973565e194c094d163d2fd0873364d0e36951806/ui/src/app/tsconfig.json#L6)
    - this new module format did require updating some legacy code that used the [`import x = require('x')` syntax](https://www.typescriptlang.org/docs/handbook/modules/reference.html#export--and-import--require)
      - update it to use what are nowadays just regular ESM imports
      - remaining `require` usage is all for CSS/SCSS and a handful of packages that have no types (or at least that we haven't imported an `@types/` package for)
        - those should be converted in the future too, but nbd right now
    - there are some new TS/Webpack warnings that pop up as a result of this new module format
      - they're warnings, so not causing any errors just yet -- they should be worked on in the future though
      - seemingly all of these are for exports of types -- they should be moved to the [newer `export type` syntax](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export)
        - problematically, updating to `export type` seems to cause errors in legacy `tslint`... so we may be blocked until that is swapped out for `eslint`
  - add a `default` `export` to `Reports` because this is (still) currently required for lazy loading

- minor stylistic changes while at it
  - consts assigned to anonymous functions -> named functions for better tracing
  - new line between external imports and internal imports for consistency

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

still more code splitting that can be done

Maybe do it in one PR?

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 22, 2023

The monaco-editor ones are a good bit more complex, so idk that I'll get to them soon, as I wrote above.
I put the PR out so we can get some improvements sooner rather than later.

It also establishes the build system and patterns for code-splitting so that other contributors can more easily add code-splitting as well. This PR has to be first given the required build system changes.

@terrytangyuan
Copy link
Member

Same comment for this PR #12062 (comment)

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 22, 2023

Same comment for this PR #12062 (comment)

I would not call these changes "trivial" in any definition of that word...
They in fact took me several hours to do from bundle analysis, to code-splitting, to build fixes, to testing, and then iteration (heck, bundling for prod on its own can take minutes)... Even in between my own comments on #12059 and here, you can see that there is a multi-hour gap...

There is no benefit to a large PR, but smaller PRs and commits are well regarded as best practice.
Notably, I've even shown you some examples before in this very codebase where large PRs made things incredibly difficult to trace (and harder to review, among other downsides).
I don't think it's a good idea to intentionally make things more difficult, and without a good reason at that.

@terrytangyuan
Copy link
Member

Sorry I didn't mean to apply exactly the same comment from the other PR. I only meant to say the first part of that comment, "I would appreciate a combined PR". While you are already at this, it might be a good idea to apply the same optimization for more components. I definitely appreciate your time and efforts spent on this.

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 22, 2023

While you are already at this, it might be a good idea to apply the same optimization for more components.

If it were as straightforward as these, I would have done so already 😕 As I mentioned above, given that there are complexities in other components, idk that I will get to them timely, as I mentioned above.
Or, in agile terminology, there are more "unknowns" others (in the code, or in the testing to determine whether a code-split is worthwhile, etc). I very intentionally did the two with the least unknowns here to first get code-splitting working correctly at least and the build system set up properly for it. These are also the most obvious optimizations given how infrequently these pages are used and how isolated their deps are (other components are used across multiple pages or used quite frequently, so there's a bit more care to be taken with them and more testing, e.g. prefetching vs preloading, size vs performance, etc)

@agilgur5
Copy link
Member Author

You also haven't really answered the question of "why wait?".
I don't see any pros to holding off on merging this for the sole purpose of waiting for even more optimizations, only cons. Any optimization is better than the current status

@terrytangyuan
Copy link
Member

terrytangyuan commented Oct 23, 2023

You also haven't really answered the question of "why wait?".

Where did you post that question? My apologies if I have missed anything.

Just waiting for other UI experts to review

Copy link
Member

@toyamagu-2021 toyamagu-2021 left a comment

Choose a reason for hiding this comment

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

LGTM. I respect the contributions of both of you.

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

3 participants