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

Proposal: Switch bundling to ESBuild #124

Conversation

colin-grant-work
Copy link

@colin-grant-work colin-grant-work commented Apr 10, 2024

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

@colin-grant-work colin-grant-work force-pushed the chore/es-build branch 2 times, most recently from c3b40c3 to d0ddc45 Compare April 10, 2024 19:53
 - move styles into src and use imports to include
 - add manual shim for Buffer
 - update CI checks to use tsc for typechecking
 - clean up vscodeignore
@thegecko
Copy link
Contributor

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
Copy link
Author

colin-grant-work commented Apr 11, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants