-
Notifications
You must be signed in to change notification settings - Fork 722
Esbuild instead of webpack for faster builds #1776
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
Conversation
…lugins Signed-off-by: 7HR4IZ3 <[email protected]>
…tracking Signed-off-by: 7HR4IZ3 <[email protected]>
… CSS processing Signed-off-by: 7HR4IZ3 <[email protected]>
Summary of ChangesHello @7HR4IZ3, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major overhaul of the project's build infrastructure by transitioning from Webpack to Esbuild. The primary goal is to enhance build performance and developer experience. Additionally, the plugin loading system has been made more resilient by incorporating timeout mechanisms and better handling of failed plugin loads, ensuring a more stable application startup. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request successfully replaces Webpack with esbuild, which should significantly improve build times. The introduction of a dedicated esbuild configuration script is well-structured. Additionally, the changes to the plugin loading mechanism, such as adding timeouts and tracking broken plugins, are excellent improvements for the application's robustness. I have a couple of suggestions regarding the new build configuration and the plugin loading logic to enhance stability and prevent potential issues.
| [onPluginsLoadCompleteCallback]() { | ||
| for (const key in this.#pluginWatchers) { | ||
| this.#pluginWatchers[key].reject(); | ||
| } | ||
| } |
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 method correctly rejects promises for plugins that didn't load. However, there are a couple of improvements that could be made:
- It's good practice to reject promises with an
Errorobject to provide more context for debugging. - The
#pluginWatchersobject is not cleared after rejecting the promises. This could lead to a memory leak, as watchers for failed plugins will persist.
I suggest modifying this method to include an error message and to clear the watchers.
[onPluginsLoadCompleteCallback]() {
for (const pluginId in this.#pluginWatchers) {
this.#pluginWatchers[pluginId].reject(
new Error(`Plugin '${pluginId}' failed to load.`),
);
}
this.#pluginWatchers = {};
}| build.onResolve({ filter: /^crypto$/ }, () => ({ | ||
| path: "crypto", | ||
| namespace: emptyNamespace, | ||
| })); | ||
|
|
||
| build.onLoad({ filter: /.*/, namespace: emptyNamespace }, () => ({ | ||
| contents: "export default {};", | ||
| loader: "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.
Stubbing the Node.js crypto module is a common practice for browser bundles, but it can lead to runtime errors if any dependency strictly requires it. While this project uses crypto-js, could you please confirm that no functionality is broken by providing an empty module for crypto? It might be safer to use a browser-compatible polyfill if any part of the application or its dependencies relies on the Node.js crypto API.
|
The Plugin Callbacks code Need to be excluded from this PR. CC: @Acode-Foundation/acode |
|
My bad, i'll recreate this |
Signed-off-by: 7HR4IZ3 [email protected]