JS Packaging and bundling with ESBuild #8792
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update transitions the frontend build tooling from webpack to esbuild, introducing a new esbuild configuration script and related plugins. It modifies asset inclusion in the main HTML template, updates worker creation logic, removes an obsolete polyfill, adjusts proxying for the dev server, and updates dependencies to support the new build process. Changes
Estimated code review effort3 (~45 minutes) Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
hotzenklotz
added
frontend
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
package.json (1)
75-84: Update package.json build scripts to use esbuildAn
esbuild_config.jsfile has been added, but the following scripts inpackage.jsonstill invoke webpack. To complete the migration, update them to call your esbuild configuration instead:• scripts.build
– Replace
"node --max-old-space-size=4096 node_modules/.bin/webpack --env production"
– With something like
"node esbuild_config.js --prod"• scripts.build-dev
– Replace
"node_modules/.bin/webpack"
– With
"node esbuild_config.js"• scripts.build-watch
– Replace
"node_modules/.bin/webpack -w"
– With
"node esbuild_config.js --watch"Please apply these updates in this PR (or let me know if you plan to ship them in a follow-up).
frontend/javascripts/viewer/workers/comlink_wrapper.ts (1)
52-57: Fix undefined reference to WorkerClass.The fallback code references
WorkerClasswhich is no longer a parameter. This will cause a runtime error in Node.js contexts.Apply this fix:
if (wrap == null) { // In a node context (e.g., when executing tests), we don't create web workers which is why // we can simply return the input function here. // @ts-ignore - return WorkerClass; + return workerUrl as any; }
🧹 Nitpick comments (4)
process_shim.js (1)
1-21: LGTM! Process shim correctly provides Node.js compatibility.The implementation properly handles cross-environment compatibility by:
- Using the standard
process/browserpackage- Setting up global fallbacks for different JavaScript environments
- Providing both named and default exports for flexibility
Consider consolidating the global assignment logic for cleaner code:
-// Make it available globally -if (typeof globalThis !== 'undefined') { - globalThis.process = process; -} else if (typeof window !== 'undefined') { - window.process = process; -} else if (typeof global !== 'undefined') { - global.process = process; -} +// Make it available globally +const globalScope = globalThis ?? window ?? global; +if (globalScope) { + globalScope.process = process; +}frontend/javascripts/viewer/workers/comlink_wrapper.ts (1)
39-45: Update outdated webpack reference in comment.The comment mentions webpack but this code now works with esbuild's worker plugin.
Update the comment to reflect the current esbuild context:
-// Worker modules export bare functions, but webpack turns these into Worker classes which need to be +// Worker modules export bare functions, but esbuild's worker plugin generates URLs which need to be // instantiated first. -// To ensure that code always executes the necessary instantiation, we cheat a bit with the typing in the following code. +// To ensure that code always uses the correct worker URL, we cheat a bit with the typing in the following code.tools/proxy/proxy.js (1)
127-135: Consider renaming function to reflect esbuild usage.The URL rewriting logic is correct and handles the path transformation properly. However, the function name
toWebpackDevis now misleading since it handles esbuild requests.Consider renaming for clarity:
-function toWebpackDev(req, res) { +function toEsbuildDev(req, res) {And update the route handler on line 150:
-app.all("/assets/bundle/*", toWebpackDev); +app.all("/assets/bundle/*", toEsbuildDev);esbuild_config.js (1)
108-157: Fix unused parameter flagged by static analysis.The proto plugin logic is sound, but there's an unused parameter that should be addressed.
- root.resolvePath = (origin, target) => path.resolve(protoRoot, target); + root.resolvePath = (_origin, target) => path.resolve(protoRoot, target);This addresses the static analysis warning about the unused
originparameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
app/views/main.scala.html(2 hunks)esbuild_config.js(1 hunks)frontend/javascripts/test/_ava_polyfill_provider.ts(0 hunks)frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx(1 hunks)frontend/javascripts/viewer/workers/comlink_wrapper.ts(2 hunks)package.json(3 hunks)process_shim.js(1 hunks)tools/proxy/proxy.js(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/test/_ava_polyfill_provider.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the `webknossos` project, changelog entries in `CHANGELOG.unreleased.md` should be concise and avoid unnecessary detailed explanations.
frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx (1)
Learnt from: knollengewaechs
PR: #8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use useRef instead of useState to ensure the event handlers always have access to the current value. This is because event listeners added via addEventListener would capture the initial state value if useState were used.
🪛 Biome (1.9.4)
esbuild_config.js
[error] 141-141: This parameter is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend origin with an underscore.
(lint/correctness/noUnusedVariables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (12)
package.json (2)
13-13: LGTM! The esbuild plugins support the migration goals.The added plugins provide essential functionality for the esbuild migration:
- Worker plugin for web worker handling
- Copy plugin for asset management
- Less CSS preprocessing
- Node.js polyfills for browser compatibility
Also applies to: 46-48
142-142: LGTM! Version update aligns with import style change.The beautiful-react-hooks version update from ^3.11.1 to ^3.12 supports the import style change seen in the comment tab view file.
frontend/javascripts/viewer/view/right-border-tabs/comment_tab/comment_tab_view.tsx (1)
12-12: LGTM! Import style change aligns with dependency update.The change from named import to direct path import (
beautiful-react-hooks/useLifecycle) is consistent with the package.json update to version ^3.12 and supports better tree-shaking.app/views/main.scala.html (2)
48-48: LGTM! Adding type="module" supports ES module usage.The addition of
type="module"to the main.js script tag correctly enables ES module loading, which aligns with modern JavaScript bundling practices.
30-35: Remove obsolete vendor asset linksVerified that the esbuild configuration bundles all dependencies into the main bundle (with
splitting: truefor dynamic imports) and inlines CSS imports by default—no standalonevendors~main.cssorvendors~main.jsis produced. You can safely delete the commented-out vendor links inapp/views/main.scala.html:• Lines 30–35: remove the
<link …vendors~main.css…/>block
• Line 47: remove the corresponding<script …vendors~main.js…/>if still presentfrontend/javascripts/viewer/workers/comlink_wrapper.ts (1)
50-51: LGTM! URL-based worker creation aligns with esbuild approach.The change from class-based to URL-based worker instantiation correctly supports esbuild's worker plugin that generates worker bundle URLs.
Also applies to: 59-62
tools/proxy/proxy.js (1)
38-42: LGTM! Process spawn configuration looks correct.The esbuild process configuration properly uses the watch flag and port argument, with appropriate environment setup.
esbuild_config.js (5)
1-24: LGTM! Dependencies and configuration setup looks solid.The path resolution, target browsers, and plugin imports are well-structured. The commented out plugins suggest careful consideration of what's needed.
26-106: Excellent custom worker plugin implementation.The worker plugin handles the complex task of bundling workers separately with proper error handling and logging. The approach of using virtual modules for worker URLs is elegant.
A few observations:
- The async worker building in
onEndis well-implemented- Error handling propagates build failures correctly
- IIFE format for workers is appropriate for browser compatibility
159-227: Comprehensive build configuration with good practices.The build options are well-configured with appropriate settings for both development and production modes. The external dependencies, aliases, and loader configurations look correct.
Key strengths:
- Proper source map configuration for dev/prod
- Appropriate chunk naming with hashes
- Good external dependency handling
- Comprehensive file loaders
228-264: Excellent development server and build mode handling.The watch mode implementation with development server and graceful shutdown is well-implemented. Production mode properly generates metafiles for analysis.
The development server logging and graceful shutdown handling are particularly well done.
268-280: Clean CLI argument parsing.The command-line interface is simple and effective for the supported flags.
hotzenklotz
changed the title
Lighthouse Performance Audit on https://master.webknossos.xyz;
Lighthouse Performance Audit on https://esbuildbundling.webknossos.xyz/
|
…s into esbuild-bundling
Transpile, package and bundle WK with ESBuild instead of webpack. Esbuild removes a lot more clutter from our node_modules resulting in smaller packages, up to 40% overall. Examples:
Antd: 1.5MB --> 660kb
Threejs: 900+kb --> 513kb
Chrome Lighthouse Performance Audit also measures clear improvements:
(Explore this graph interactively at https://esbuild.github.io/analyze/ with this file:
metafile.json)
I tested with the following browsers without any issues:
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)