istio.io icon indicating copy to clipboard operation
istio.io copied to clipboard

LFX'25:Update the pipeline to achieve clean build without any errors

Open Ajay-singh1 opened this issue 6 months ago • 5 comments

Description

This PR replaces Babel with esbuild as the build tool for this repository. The main goals of this is to improve build performance, reduce configuration complexity, modernize the toolchain, and generate cleaner builds.

Overview

Babel has served as a popular JavaScript transpiler for years, but it comes with some downsides in the context of modern library development:

  • Performance bottlenecks: Babel is significantly slower, especially on large codebases or repeated builds
  • Maintenance concerns: Many Babel plugins and presets are either deprecated, minimally maintained, or lag behind the evolving JavaScript standard.
  • Unnecessary complexity: Babel's plugin-based architecture leads to verbose and often brittle configurations

esbuild, on the other hand, is designed for speed and simplicity, with modern JavaScript/TypeScript support and built-in minification and bundling.

Technical Changes

Removed

  • Babel related dependencies
    • @babel/core
    • @babel/preset-env
    • Anyother babel related dependency

Added

  • esbuildas a dev dependency
  • A new build script using esbuild, capable of:
    • Transpiling modern JavaScript (ES6+) to target environments
    • Minimification of Javascript files to achieve faster builds

Testing

  1. The output structure and exports matches with previous Babel generated output
  2. The site is build successfully without having any errors or warnings

Result

  1. Simplified build setup with fewer and less boilerplate dependencies
  2. More future-proof and modern toolchain

Reviewers

  • [ ] Ambient
  • [ ] Docs
  • [ ] Installation
  • [ ] Networking
  • [ ] Performance and Scalability
  • [ ] Extensions and Telemetry
  • [ ] Security
  • [ ] Test and Release
  • [ ] User Experience
  • [ ] Developer Infrastructure
  • [ ] Localization/Translation

Ajay-singh1 avatar Jun 12 '25 05:06 Ajay-singh1

Great initiative. The new modules would need to be in the build container before the lint will pass. I don't have enough context to say yes or no without doing a bunch more work.

Can you comment on (or show a diff of) the before/after of the generated JS?

You're asking for approval to merge something that transpiles/touches the site JavaScript; there's no point in showing a video which scrolls a bunch of random pages, we would need to see the before/after for things that are executed by these scripts.

(p.s. generating the site takes ~20 seconds on my laptop. Do I read right that yours takes 440 seconds - 20 times longer? We need to do something about that!)

craigbox avatar Jun 12 '25 08:06 craigbox

I found that this PR is causing the headerAnimation script and the expandable list functionality to break. If you check the js folder, the headerAnimation.js file is likely missing from the build. Additionally, a few other files might also be missing from the js directory. If you compare the builds generated using Babel and esbuild, you'll notice that the file structures differ significantly.

AritraDey-Dev avatar Jun 12 '25 08:06 AritraDey-Dev

@craigbox The work is not completed yet I'll ping you when it does. @AritraDey-Dev Yeah I agree with you that the file structure is different but I have resolved this I will push the changes in the meantime. Thanks

Ajay-singh1 avatar Jun 12 '25 09:06 Ajay-singh1

@craigbox This PR is up for review , after this PR is merged istio/tools PR .Hopefully the lint will pass here.

Ajay-singh1 avatar Jun 17 '25 02:06 Ajay-singh1

https://github.com/istio/tools/pull/3222 LGTM but I'm not in the reviewer set. Daniel is, so I think that will go through today his time.

craigbox avatar Jun 17 '25 09:06 craigbox

@dhawton Can you kindly also review this PR and share your comments?It would be really helpful.

Ajay-singh1 avatar Jun 17 '25 10:06 Ajay-singh1

Thanks @dhawton and @craigbox for the detailed review I have done the necessary changes. @keithmattix Can you kindly also review this PR? just to ensure everything looks good.

Ajay-singh1 avatar Jun 19 '25 15:06 Ajay-singh1

Nice catch! The error happened because of leftover compiled files from the previous build you won't get this error before because how babel handles files is different then how esbuild handles them.

BTW this is a quick fix.

Ajay-singh1 avatar Jun 20 '25 04:06 Ajay-singh1

I just gave an example that running make serve is throwing errors. If you noticed, it's also throwing errors for other make targets. Adding the clean target to every make command is not a good solution just to resolve the errors.If you add clean to every make target, it unnecessarily takes time to clean before performing the actual task.

Also, after you build it, you’ll see a bunch of red squiggly lines — this shouldn’t happen. I'm also not sure why you added so many import and export statements. Previously, when we were using a Babel-based build, the functions or variables that needed to be compiled globally were already there.

AritraDey-Dev avatar Jun 22 '25 20:06 AritraDey-Dev

I think it's reasonable for a build to clean up any temporary files after using them?

However the entrypoint.js file should not contain reference to tmp files that aren't normally there:

import "../../tmp/js/constants.js";
import "../../tmp/js/utils.js";
import "../../tmp/js/feedback.js";

craigbox avatar Jun 23 '25 00:06 craigbox

@AritraDey-Dev esbuild isn't a transpiler like babel it also does bundling of files so this has to happen anyways.Regarding the export and imports I already told Craig that the repo is using global legacy code and converting them to ES6 modules is a good idea.Legacy code was used before ES6 and it was a standard then.

It is obvious to clean temporary files before each build so that isn't a major issue here.

For ex:- Try to compile a .ts file with estarget set to es6 you will get errors.

Ajay-singh1 avatar Jun 23 '25 11:06 Ajay-singh1

In this case the entrypoint file should be generated by the build process IMO.

It's just a listing of all the files to compile. Better we don't use one at all?

craigbox avatar Jun 24 '25 03:06 craigbox

Yes you are correct these are for compilation but having an entrypoint.js file seems like a good idea as Daniel mentioned it would be convenient for future maintainers and contributors to look into this file and get the idea of how things are working.

Ajay-singh1 avatar Jun 24 '25 05:06 Ajay-singh1

@dhawton @craigbox Can we please merge this PR if everything looks good?

Ajay-singh1 avatar Jun 28 '25 11:06 Ajay-singh1

I'm still looking for a change in the entrypoint file. I had assumed it would contain links to the existing scripts, but it seems it has to have links to ../tmp/ to work. Right now, someone will see it, it has links to files that don't exist until the build time, and you have to dig through the build script to figure out why.

Given that I think it's best that it is generated when the build is done and removed afterwards.

craigbox avatar Jul 01 '25 11:07 craigbox

I'll look into this and update you.

Ajay-singh1 avatar Jul 01 '25 11:07 Ajay-singh1

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

istio-testing avatar Jul 02 '25 11:07 istio-testing

@dhawton Thanks! I'll push the changes ASAP.

Ajay-singh1 avatar Jul 02 '25 15:07 Ajay-singh1