search

eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

Proposal: Switch bundling to ESBuild #124

Closed colin-grant-work closed 5 months ago

colin-grant-work commented 5 months ago

What it does

This PR switches bundling from Webpack to ESBuild.

Pros:

  1. Much, much faster: On my machine, yarn build runs in 0.41s vs. 24 seconds with Webpack.
  2. Smaller bundle: dist (with source maps) about 12% smaller.
  3. More transparent bundle: everything is in dist; nothing additional packaged from node_modules.
  4. Fewer dependencies.

Cons:

  1. Type checking is not part of build: added separate types script to check with tsc.
  2. Can struggle with dynamic imports (but so can Webpack). I have run afoul of this problem in the past, but there don't appear to be any dynamic require calls in the bundled code in this repo.
  3. Because type checking isn't part of the build, running the watch task won't produce error output if you introduce errors in development. If that's an important part of people's workflow, we can run tsc --watch in parallel with ESBuild's watch.

How to test

  1. Local smoke tests - do all styles & icons in webview appear correct?
  2. Package and install + smoke tests - ensure that we're not cheating and using local files that wouldn't be packaged.

    Pay special attention to browser plugin use case - I'm less familiar with its behavior, as we use primarily the desktop package.

Review checklist

Reminder for reviewers

thegecko commented 5 months ago

Introducing another build system (considering my team maintains a lot of extensions we aren't going to immediately switch to esbuild) does bring some overhead with learning and context switching.

We could of course consider moving all extensions to use this approach but would need solutions where we currently use polyfills, stubs, native library support and library substitution for Web/Desktop

This is quite a large architectural change so needs careful consideration IMO

colin-grant-work commented 5 months ago

This is quite a large architectural change so needs careful consideration IMO

@thegecko, fair enough. I don't think we need to rush into it. We're in the opposite boat: we've migrated most of our extensions over to ESBuild (and we're loving it, for the most part 😄), but as long as there's a build system that works, I'm not unhappy. I'll close this for now, but you can consider it a friendly recommendation to try out ESBuild and see how it works for you :-).

thegecko commented 4 months ago

Thanks!

TBF I'm quite keen to try esbuild. I'll take this as intended and we can investigate switching over from a more holistic point of view.