build(ui): improve performance with esbuild-loader

[agilgur5]

Follow-up to #11628

Motivation

• similar to Argo CD and many other projects, we can use esbuild instead of TS etc to significantly speed up build times
  • (as esbuild is written in Go and compiled instead of using interpreted TS/JS)
  • we don't use any Babel or TS plugins, so I think this codebase is a good candidate for that
  • it also simplifies the build process as well, removing & replacing many other deps
  • caveats:
    • esbuild does not do type-checking, so we have to run tsc --noEmit as part of the lint phase
    • esbuild-loader does not currently support React Fast Refresh (c.f. about hmr react-refresh/babel  privatenumber/esbuild-loader#250, Using with esbuild-loader? pmmmwh/react-refresh-webpack-plugin#366)
      • react-hot-loader was replaced by React Fast Refresh and is deprecated / hasn't received an update in 2 years
      • that being said, esbuild is significantly faster than using Babel or TS for builds, so hot reloading is nice but not necessary
        • also hot reloading can be buggy
        • plus, in my experience, the dev build often falls back to a full page refresh anyway

• resolve various warnings in the build, during the install, and in the console
• replace multiple deps with 1

Modifications

• remove babel-loader, ts-loader, react-hot-loader, and source-map-loader
  • note that source-map-loader is still present in the yarn.lock as it is used under-the-hood by other deps
    • but the warnings from directly using source-map-loader are no longer present in the build
    • and the bundle size shrank a bit, seemingly bc there is no autolinker etc dep in the bundle, but potentially other reasons (I did not check in-depth)

• add esbuild-loader

• add tsc --noEmit to the lint script

• add isolatedModules config to ensure compatibility with esbuild's compiler

  • since it has no type-checking, it can't do full program analysis, meaning there are certain types it doesn't support

  • same as when using Babel to compile TS

  • fix some resulting type-check errors:

    • upgrade argo-ui to use export type syntax: fix: Use type export for interface reexports argo-ui#552
    • pin types-ramda due to requiring newer TS: build(ui): improve performance with esbuild-loader #12516 (comment)
    • workaround: add Intl typings missing in older TS: a4511f6

• remove mdl.hot etc usage of react-hot-loader

Related config / performance changes

• further optimize dev builds by using cheaper eval sourcemaps as recommended by Webpack for performance

  • prod builds should always have highest quality sourcemaps, so do not change those
  • this was also discussed in chore: remove ui source map for production build #11716 (comment)

• remove unused react-paginate exclude

  • it's not a dep we currently use; it doesn't exist in the lockfile
    • (nor in argo-ui's lockfile, of which all prod deps would be in Workflows's lockfile as well)

Verification

1. yarn build works fine
2. make start UI=true works fine and UI seems to function as expected

Performance Testing

some simple performance testing when running natively on macOS, taken directly from Webpack's reported stats:

• before:
  • prod build: ~65s
  • dev build: ~22s
  • dev rebuild: ~1.8s
• after:
  • prod build: ~50s (~25% faster)
  • dev build: ~16s (~25% faster)
  • dev rebuild: ~.8s (~50% faster)

Future Work

these are not as good improvements as I would have liked, which mean there are bigger bottlenecks elsewhere

• e.g. Webpack itself (analysis, source maps, running loaders), sass-loader, etc
  • sass has been slow in the past and is ripe to be replaced with more modern PostCSS as well
  • switching to esbuild directly may further improve these stats
    • i.e. instead of Webpack + esbuild-loader
• independently building argo-ui instead of as part of the Workflows (and CD) builds per argo-ui: future state of this repo argo-ui#453
• optimizing more deps per UI: Code Split larger and/or less-used paths for smaller initial bundle #12059 and related comments, such as removing moment, consolidating similar deps, etc

[agilgur5]

Gah, this already got a merge conflict with a Dependabot PR #12511. Adding prioritized-review label to this and #12487 as a result

[agilgur5]

Oh, interesting, due to the new isolatedModules config, the build is failing due to missing import type. i.e. this PR actually depends on #12514

[isubasinghe]

Nice work, this is so much faster.
Works as expected

[terrytangyuan]

Build failed

[isubasinghe]

@terrytangyuan you need to merge this one first: #12514

[agilgur5]

Fixed another merge conflict in the yarn.lock. Still waiting for #12514 to be merged so that this can be unblocked and merged

[juliev0]

@isubasinghe are you comfortable being the Assignee for this one, so you may be able to merge it once you officially have access to?

[agilgur5]

Merged main since #12514 was merged, but getting a test failure in argo-ui now 😕 . Interestingly enough, the build passes fine (which I had tested), it's only the tests' build that's failing, I think because the tooling is different (ts-jest uses the TS compiler directly, esbuild-loader uses esbuild)?

I was trying to partly workaround a CSS side-effect import in #12158 for similar reasons to this, but otherwise this requires a few changes upstream in argo-ui (where my PRs have been open even longer 😕)

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

[juliev0]

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

Okay, yes!

[isubasinghe]

@isubasinghe are you comfortable being the Assignee for this one, so you may be able to merge it once you officially have access to?

Yup happy to merge when the tests pass

[agilgur5]

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

I might be able to get this working again now that argoproj/argo-ui#509 and argoproj/argo-ui#552 have been merged

[agilgur5]

Guh, this is now failing to typecheck due to another dep, this time very transitive: ramda/ramda#3415 (comment), swagger-api/swagger-js#3176, swagger-api/apidom#3279

[agilgur5]

Ok had to fix that by pinning types-ramda as it requires TS v5 per ramda/ramda#3415 (comment) and swagger-api/swagger-js#3176 (comment)

[agilgur5]

Verification

1. yarn build works fine
2. make start UI=true works fine and UI seems to function as expected

Re-verified, seems to all be fine

[agilgur5]

I'm gonna go ahead and merge this since I only made the changes necessary to fix CI since this was last approved. It also fixes a bunch of warnings and improves performance etc, so it's pretty darn nice to have sooner rather than later