-
Notifications
You must be signed in to change notification settings - Fork 1.3k
make esbuild the default builder for the web app #46062
Conversation
cdb76ad
to
12cad92
Compare
12cad92
to
e76ae00
Compare
Hooray! evanw/esbuild#1458 was just fixed. So, we can proceed with using esbuild instead of Webpack. (I have heard only very positive sentiment about this that I can recall, and I know half or more of our devs are already using esbuild in local dev. If there are reasons to NOT move forward with this, including any that I may have forgotten about, please speak up!) |
Codenotify: Notifying subscribers in OWNERS files for diff debb6ff...c86a33d. No notifications. |
This is essentially blocked on evanw/esbuild#1458 because: - That PR is needed to make tree-shaking work with esbuild in our app. - We could just use esbuild for dev (where tree-shaking isn't as necessary) and Webpack in prod, but we don't want to use a different builder in dev vs. prod. - Using an esbuild fork is cumbersome, since we'd also need to build the `esbuild` binaries for various platforms.
e76ae00
to
c86a33d
Compare
@sqs, it's nice that esbuild doesn't have any blockers to be a full-fledged builder for our project. A few things that I can say here in favour of using webpack is that with webpack; we're getting very deep dive bundle reports that help a lot in bundle investigation and debugging (and we actually use it in PRs; you probably have seen these size comments in PRs about initial and async chunks sizes) I'm not sure about this, but I think with bazel adoption, webpack compilation should become much faster for non-big migrations PRs. Also I think it's still useful sometimes (especially when you're doing layout work to have hot reload DX when you don't need to load big chunks of data every time you change your CSS) |
@vovakulikov, got it, thank you. Those are indeed good things about Webpack (the bundle analyzer, hot reloading, and potential for perf improvements with Bazel). We can get a bundle analyzer with esbuild. I don't know about hot reload. Overall, do you think that we should stay with Webpack? I am speaking from my personal experience of having felt Webpack pain for like ~7 years: ~30-60s restart times, long reload times, and multi-minute build times. esbuild seems SO SO SO SO SO much better. esbuild does those things in ~500ms-3s. Is my Webpack experience an outlier? Have you tried esbuild? |
Oh, just to be clear, I'm not saying that we shouldn't replace it with esbuild by default, but sometimes hot reloading might give you a really good experience (of course, it depends on the type of work you're doing), so I think I still would like to have webpack as a possible option, I tried esbuild and it's indeed much much faster (I think webpack is outdated and obsolete, too big and complex to improve performance there I guess)
Hmm, this is strange; usually, with disk cache on my machine, it usually takes around 10-15 seconds to start serving files, but with esbuild it takes 1-2s, so I still see huge benefits from using esbuild instead by default. |
@vovakulikov Cool, makes sense. I bet I could write a script that translates the esbuild metafile to the |
Made a separate PR that just upgrades esbuild and enables tree-shaking and does NOT make it the default builder: https://github.com/sourcegraph/sourcegraph/pull/47520. We can get that merged soon and try it out more before deciding whether to make esbuild the default. |
This is awesome! My personal opinion is that we should go ahead with this. Webpack needs to go, not alone because it's slow but also because it needs a lot of configuration. I've been setting up the JetBrains project with esbuild and it was just a lot easier than the VSCode config for Webpack that we had. Same thing when I paired with @burmudar on the backstage scaffolding. I personally think that Vite (which uses esbuild for some parts) would be a better compromise for us but it takes time to switch the bundler and esbuild already works for our setup? I also think that this is easily revert-able and we can also switch to Vite or anything else in less then a day if we do have some real blockers. I’m actually really concerned about the current situation where there's a difference between the bundle that we develop locally against (I’m using esbuild locally and so are many other devs) and the production build. I've been bitten by differences in minified/non-minified builds enough before. There are two valid concerns with esbuild though:
|
The good part of statoscope (which differentiates it from all other tools) is the ability to answer questions about why something was included in my bundle and track the exact import path, so it's not just a bundle-size tool. For example, with webpack bundle analyzer or esbuild analyze, there is no way to figure out which import you should change in order to remove deps from the chunk. (@sqs I actually don't think that esbuild exposes enough information to have the same functionality from the converted esbuild stats file) I don't want to defence HMR, but just to be fair, I need to say that I didn't have many situations with dirty state, probably because, in most cases, I do my work inside react call tree, and everything is fine there, but code exploration team works a lot with external stores like code mirror state (and here I agree HRM may work incorrectly there in many cases, but it doesn't mean that it works bad for all consumers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit late to the party!
I've taken the time to go through both configuration files and our CI jobs. Before fully transitioning to esbuild, we need to ensure the following aspects are reviewed and addressed (some items may not require any action other than a functionality check):
Configuration review
- Compression plugin for production bundles. Our Webpack bundle relies on brotli compression in production.
- Split chunks optimizations for production bundles. Review if we can mirror our setup in Webpack configuration for long-term caching of key dependencies like React.
- Mirror the DefinePlugin configuration. Currently, not all required environment variables are injected into the client bundle.
- Support the embed entry point required for standalone notebook pages.
- Integrate with browserslist or replace it with a new approach. This won't be necessary if we complete the Bazel migration first. More details on this topic are mentioned below.
- Maintain code coverage support for our integration tests (currently achieved via the Babel plugin) or decide to discontinue it.
- Analyze and compare the production bundle outputs from both esbuild and Webpack, potentially fine-tuning the esbuild configuration based on the results.
- Implement long-term hashing for production bundles.
- Ensure source maps are generated correctly for production bundles.
- Verify that integration tests work with esbuild production bundles, including any necessary manual plumbing. Ensure that source maps are available in integration tests.
- Enable the bundle analyzer and ensure compatibility with Statoscope or a similar tool, which provides reasons for module inclusion in chunks. This step is crucial, as it helps prevent difficulties in debugging and fixing issues related to bundle size changes.
- Low priority: Sentry source maps plugin support.
CI integration
- Integrate with the bundlesize check. It probably works out of the box.
- Integrate with Statoscope reports or a similar tool.
- Integrate with Bazel.
Bazel
In Bazel, the transpilation process is separate and cacheable from bundling, which means that esbuild will not handle it directly. Instead, esbuild will only do bundling with precompiled CSS and JS files. To accommodate this, we need to update the configuration accordingly. For instance, with Webpack, we created a dedicated config file webpack.bazel.config.js
for now. We would need to utilize the rules_esbuild maintained by the Aspect.dev team, who are assisting us with Bazel integration.
Currently, we rely on Babel for TypeScript transpilation in our Bazel configuration, which is slow compared to esbuild. We can eliminate this cacheable transpilation layer and depend on esbuild directly because it's so fast. Another option would be to use swc, which supports browserslist, offers significantly faster performance than Babel, and has a Bazel rule that we can use. I plan to discuss this further with our Bazel contractors to ensure we understand the trade-offs.
HMR
Hot reloading can greatly improve the developer experience by shortening the feedback cycle, especially for UI-heavy work. While HMR may not be essential for all types of changes, it's up to each developer to leverage this functionality according to their needs and preferences. Even though I will miss this feature, it's clear that the esbuild speed-up outweighs HMR benefits.
Many frontend developers appreciate Vite because it offers the best of both worlds, combining HMR with esbuild's performance in development. Sadly, there are no up-to-date Bazel-Vite rules available in the open source. The Aspect.dev team has an open GitHub issue about Vite support in their repo, but they do not have plans to work on it now. So our best bet in the near future is to migrate to esbuild to keep our setup consistent between CI and local development.
Bottom line
Clearly, folks love esbuild, and we would improve our DX by switching to it. I'm on board with that!
That said, we need to be careful while making this change. We should ensure everything is tested, our customers still get highly optimized artifacts, and the integration with our CI pipeline works as expected. This should be a focused effort over a few weeks, not something we squeeze in when we have a spare minute.
During this job-fair iteration, we've integrated Bazel with Webpack and will roll it out to everybody soon. The bundling time has improved, but obviously, it's still slow compared to esbuild (~15s start on startup), and recompilation time is untested yet. I propose:
- Let's finish the Bazel integration with the current setup (Webpack), test it locally and on CI. We can keep using esbuild for local development in the meantime.
- Once the Bazel pipeline is up and running, invest resources (job-fair project for that?) into checking all the remaining boxes for the esbuild config and integrating it with Bazel.
Let me know what you think!
P.S. Big thanks to @sqs for pushing this forward for a long time and getting us so close to the finish line!
P.S. [2] I didn't expect the comment to be that long, a big part of it will go into our Bazel docs 😜
@valerybugakov Thanks for the in-depth review! I agree with what you said about being careful. Some inline thoughts to some points you raised:
Can you clarify what you mean by this? How does the webpack bundle care about brotli compression? Isn't this the job of the http server?
I think this is an overrated performance improvements. It only really matters for dotcom and S2 (where we deploy regularly) and for our customers who might only update once every x months, this might not be noticeable at all. That said, sure if we can have it, we'll take it 🙂
I added this for the app build after we had our first caching issues. Anything in particular you meant here?
Do you think there's an issue here or is this just something to be cautious of? Sourcemaps are enabled in esbuild and they are normally linked to from the source files automatically so I don't anticipate any issues here.
I’m not sure if it's helpful and if you've looked into it already, @valerybugakov, but Remix has added HMR to esbuild https://github.com/remix-run/remix/pull/5259/files#diff-e7b06fec694904307e32408c2b6dd984aefee4f2b3f51442803f957db6a288bc -- Perhaps there's something about this setup that we can replicate in our setup.
😭 Unrelated to Valery's response, I wonder how we can make a decision wether we want to move forward with the migration to esbuild or not. There's a tax we pay right now in having to maintain two different build systems and there's a mismatch between the bundle that some developers see in their development environment vs. what we ship to browsers which can yield to unforeseen issues. |
Sorry if this is a stupid comment, but what do we expect from webpack with bazel in terms of performance? Will esbuild matter at that point? Seems like there's a lot of good arguments for not having two build systems, locally but also dev vs prod. |
Plus one to Erik's question above. I got impression that people suffer from tslow cold build with webpack in their local environment (I actually didn't notice this, webpack with current cache system works just fine to me, it's around 5-10 sec with cache start and around 1-2 sec in HMR rebuild) |
I think one point of friction at the moment is that the generators run before webpack can start, making this a waterfall and the generators themselves take around 8s. Perceived cold start could be reduced as well by making those generators faster. |
Closing this PR since it's been inactive for many months and it comes up when I search for open PRs with the query "JetBrains". |
Makes esbuild the default builder for the web app. You can still use
DEV_WEB_BUILDER=webpack
to use Webpack (for now).TODOs:
Test plan
e2e tests all now run against the esbuild bundle.
App preview:
Check out the client app preview documentation to learn more.