Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

make esbuild the default builder for the web app #46062

Closed
wants to merge 1 commit into from
Closed

Conversation

sqs
Copy link
Member

@sqs sqs commented Jan 3, 2023

Makes esbuild the default builder for the web app. You can still use DEV_WEB_BUILDER=webpack to use Webpack (for now).

TODOs:

  • Fix bundle size calculations? (Were these Webpack-specific?)
  • Check that the build process (in CI) uses esbuild now
  • Should we just remove Webpack altogether (for the main web build I mean), instead of keeping it around as the non-default?

Test plan

e2e tests all now run against the esbuild bundle.

App preview:

Check out the client app preview documentation to learn more.

@sqs sqs requested a review from a team January 3, 2023 07:16
@cla-bot cla-bot bot added the cla-signed label Jan 3, 2023
@sqs sqs changed the title make esbuild the default builder for the web app BLOCKED ON UPSTREAM: make esbuild the default builder for the web app Jan 3, 2023
@sqs sqs marked this pull request as ready for review February 10, 2023 19:35
@sqs
Copy link
Member Author

sqs commented Feb 10, 2023

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!)

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Feb 10, 2023

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.
@sqs sqs changed the title BLOCKED ON UPSTREAM: make esbuild the default builder for the web app make esbuild the default builder for the web app Feb 10, 2023
@sqs sqs marked this pull request as draft February 10, 2023 20:16
@vovakulikov
Copy link
Contributor

@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)

@sqs
Copy link
Member Author

sqs commented Feb 10, 2023

@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?

@vovakulikov
Copy link
Contributor

vovakulikov commented Feb 11, 2023

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)

~30-60s restart times

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.

@sqs
Copy link
Member Author

sqs commented Feb 11, 2023

@vovakulikov Cool, makes sense. I bet I could write a script that translates the esbuild metafile to the stats-[name]-[hash].json file used by statoscope.

@sqs
Copy link
Member Author

sqs commented Feb 11, 2023

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.

@philipp-spiess
Copy link
Contributor

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:

  • We don't have good observability into the bundle size => I think we'll have to fix the GitHub action that posts the summary but I actually don't found the statoscope specific format too useful either. E.g. there's https://esbuild.github.io/analyze/ which has a very nice UI as well. The metafile API should give us what we need. (It actually seems much easier to parse than the statoscope file)
  • We don't have HMR support with esbuild => I've been working with esbuild for the last few weeks and I didn't even notice this. I think I’m not trusting HMR much anyways (it's caused too many broken states for me in the past). That said, Vite would give us a much better experience in dev. 😅

@vovakulikov
Copy link
Contributor

@philipp-spiess

We don't have good observability into the bundle size => I think we'll have to fix the GitHub action that posts the summary but I actually don't found the statoscope specific format too useful either. E.g. there's https://esbuild.github.io/analyze/ which has a very nice UI as well. The metafile API should give us what we need. (It actually seems much easier to parse than the statoscope file)

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)

Copy link
Member

@valerybugakov valerybugakov left a 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:

  1. 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.
  2. 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 😜

@philipp-spiess
Copy link
Contributor

@valerybugakov Thanks for the in-depth review! I agree with what you said about being careful. Some inline thoughts to some points you raised:

Compression plugin for production bundles. Our Webpack bundle relies on brotli compression in production.

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?

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.

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 🙂

Implement long-term hashing for production bundles.

I added this for the app build after we had our first caching issues. Anything in particular you meant here?

Ensure source maps are generated correctly for production bundles.

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.

HMR

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.

Sadly, there are no up-to-date Bazel-Vite rules available in the open source.

😭


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.

@eseliger
Copy link
Member

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.

@vovakulikov
Copy link
Contributor

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)

@eseliger
Copy link
Member

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.

@olafurpg
Copy link
Member

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".

@olafurpg olafurpg closed this Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants