-
Notifications
You must be signed in to change notification settings - Fork 1.6k
LFX'25:Update the pipeline to achieve clean build without any errors #16574
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
base: master
Are you sure you want to change the base?
Conversation
Great initiative. 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!) |
I found that this PR is causing the |
@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. |
@craigbox This PR is up for review , after this PR is merged istio/tools PR .Hopefully the lint will pass here. |
@Ajay-singh1: The following test failed, say
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. I understand the commands that are listed here. |
istio/tools#3222 LGTM but I'm not in the reviewer set. Daniel is, so I think that will go through today his time. |
tmp/js/themes_init.js \ | ||
--out-file generated/js/themes_init.min.js | ||
#Entrypoint for esbuild to bundle and minify through an entrypoint.js file | ||
cat <<EOF > tmp/js/entrypoint.js |
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.
Why generate this every time and not check it in?
(This question doesn't presuppose I know the answer)
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.
esbuild needs an entrypoint for bundling the files and it does when the files are ESModules and the typescript files which is used in this repo is not ESModules but are legacy old standard type global scripts.esbuild follows the import and build a dependency graph so this temporary file is needed for esbuild for bundling the files together and minify them.
You can check the tsconfig.json the target is set to es6.
When you compile a .ts file for ex:- tsc src/ts/faq.ts --target es6
It will throw an error because the files are not following the ES6 standard.
This file is just an entrypoint and nothing else for bundling and minification.
esbuild tmp/js/headerAnimation.js \ | ||
--minify \ | ||
--sourcemap \ | ||
--target=es6 \ | ||
--outfile=generated/js/headerAnimation.min.js |
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.
Is it still relevant that this be a separate file?
(Some digging may be required to find out what it does and why it was done this way)
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.
Yeah it needs to be a seperate file otherwise it can break the site.I have to do a bunch more test.
esbuild tmp/js/themes_init.js \ | ||
--minify \ | ||
--sourcemap \ | ||
--target=es6 \ | ||
--outfile=generated/js/themes_init.min.js | ||
|
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.
same here, previously with Babel this was --minify
but not --presets minify
for some reason?
Does it still make sense to be its own thing?
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.
You run babel presets-minify
it will minify multiple files together in isolation and generate a single output file which doesn't raise global conflicts such as function , var scoping conflicts etc.esbuild does the same thing but is more advanced like it removes any unused code from the bundle through a process called as tree shaking and minifies aggressively.
declare class Popper { | ||
constructor(a: HTMLElement, b: HTMLElement, c: any); | ||
public destroy(): void; | ||
} | ||
declare var iconFile: string; | ||
// Path to the SVG icon sprite file |
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.
comments should be above the line?
@@ -11,7 +11,6 @@ | |||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
// See the License for the specific language governing permissions and | |||
// limitations under the License. | |||
|
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.
were changes such as this required to lint, or something?
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.
Yes make lint-typescript
was throwing errors.
import { listen , copyToClipboard , getById} from "./utils"; | ||
import { button , ariaLabel , mouseenter , mouseleave , click , active} from "./constants"; | ||
import { readLocalStorage } from "./themes_init"; |
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.
review top tip: I don't know anything about Typescript but it would be worth providing a little context as to why these need to be added
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.
This has to be added to make them ESModules and follow ES6 standards otherwise the .ts files will be complied as global scripts and global scripts are now legacy code which doesn't follow ES6 standard.
@dhawton Can you kindly also review this PR and share your comments?It would be really helpful. |
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:
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/core
@babel/preset-env
Anyother babel related dependency
Added
esbuild
as a dev dependencyTesting
Result
Reviewers