JS Packaging and bundling with ESBuild #8792
Conversation
📝 WalkthroughWalkthroughReplace the frontend Webpack build with an esbuild-based pipeline: add esbuild config and custom plugins, remove webpack config and polyfills, update package scripts/deps, adjust dev proxy and worker handling, and update HTML asset includes and CI step label. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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/
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tools/esbuild/protoPlugin.js (1)
46-48: CJS export inside an ESM bundle may bite you later
module.exports = …works only via ESBuild’s CJS interop shim.
For a project that emitsformat: "esm"consider emitting a real ESM default export to avoid extra wrapper code:- contents: `module.exports = ${JSON.stringify(json)};`, + contents: `export default ${JSON.stringify(json)};`,This is optional but keeps tree-shaking and typings straightforward.
tools/esbuild/workerPlugin.js (1)
69-96: Worker builds run sequentially – easy win for speed
for ... await esbuild.build()builds workers one-by-one.
Running them in parallel is safe (they are independent) and shortens total build time:await Promise.all( [...workerEntries].map(([inFile, outFile]) => esbuild.build({ ...initial, entryPoints: [inFile], outfile: path.join(outdir, outFile), /* … */ }) ) );
📜 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 (12)
.github/workflows/build_test_deploy.yml(1 hunks)app/views/main.scala.html(1 hunks)esbuild_config.js(1 hunks)frontend/javascripts/libs/DRACOLoader.ts(0 hunks)frontend/javascripts/viewer/model/helpers/reduce_reducers.ts(1 hunks)frontend/javascripts/viewer/workers/comlink_wrapper.ts(3 hunks)frontend/javascripts/viewer/workers/readme.md(1 hunks)package.json(5 hunks)tools/esbuild/protoPlugin.js(1 hunks)tools/esbuild/workerPlugin.js(1 hunks)tools/proxy/proxy.js(3 hunks)webpack.config.js(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/javascripts/libs/DRACOLoader.ts
- webpack.config.js
✅ Files skipped from review due to trivial changes (3)
- frontend/javascripts/viewer/model/helpers/reduce_reducers.ts
- frontend/javascripts/viewer/workers/readme.md
- .github/workflows/build_test_deploy.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/viewer/workers/comlink_wrapper.ts
- app/views/main.scala.html
- tools/proxy/proxy.js
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
tools/esbuild/protoPlugin.js (2)
tools/esbuild/workerPlugin.js (1)
path(21-21)esbuild_config.js (6)
path(2-2)require(12-12)require(16-16)require(19-19)require(20-20)protoPath(8-8)
⏰ 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). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (1)
esbuild_config.js (1)
101-102:/assets/images/*marked external ⇒ broken URLs in production?Declaring the glob as
externalprevents ESBuild from copying/renaming those files, yetpublicPathpoints to/assets/bundle/.
Verify that the runtime server rewrites these image URLs; otherwise images will 404 after hash-based cache busting.If images should be emitted by ESBuild, drop the pattern from
external(or handle it viacopyPlugin).
| const resolvedWorkerPath = path.resolve(baseDir, `${args.path}.ts`); | ||
| // Maintain previous behavior: strip the .worker suffix in the output filename | ||
| const workerBaseName = path.basename(args.path, ".worker"); | ||
| const workerOutputPath = `${workerBaseName}.js`; | ||
|
|
||
| workerEntries.set(resolvedWorkerPath, workerOutputPath); |
There was a problem hiding this comment.
Filename collision risk for workers
Output name is just ${basename}.js; two workers named resize.worker.ts in different folders will overwrite each other.
Consider embedding the directory or a content hash:
- const workerOutputPath = `${workerBaseName}.js`;
+ const relDir = path.relative(absWorkingDir, path.dirname(resolvedWorkerPath))
+ .replaceAll(path.sep, "_");
+ const workerOutputPath = `${relDir}-${workerBaseName}.js`;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tools/esbuild/workerPlugin.js around lines 40 to 45, the output filename for
workers is generated using only the base name, which risks collisions if
multiple workers share the same name in different directories. To fix this,
modify the output filename to include a unique identifier such as the relative
directory path or a content hash, ensuring each worker's output file is distinct
and preventing overwrites.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
GPT5 assessment/review: High‑impact changes
Expected impacts
Potential regressions & follow‑ups
Minimum supported browsers (effective)
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tools/esbuild/workerPlugin.js (2)
50-53: Filename collision risk for workers (repeat of earlier feedback)workerOutputPath =
${workerBaseName}.jsand workerUrl =${publicPath}${workerBaseName}.jswill collide across identically named workers in different directories.The diffs I suggested above resolve this by embedding a relative dir and content hash.
Also applies to: 66-66
45-61: Collision-prone output naming and non-absolute virtual module pathTwo issues here:
- workerOutputPath uses only the basename, which collides for workers with the same name in different dirs and provides no cache-busting.
- The onResolve return value uses args.path (relative/bare), which can collide across different importers because esbuild identifies modules by returned path+namespace. Returning an absolute path is recommended.
Fix by:
- Returning an absolute path (resolvedWorkerPath) for the virtual module.
- Embedding a sanitized relative directory and a short content hash into the worker filename to make it unique and cache-bust on changes.
Apply this diff:
- build.onResolve({ filter: /\.worker$/ }, (args) => { - const isBare = !args.path.startsWith(".") && !path.isAbsolute(args.path); - const baseDir = isBare ? srcRoot : args.resolveDir || absWorkingDir; - const resolvedWorkerPath = path.resolve(baseDir, `${args.path}.ts`); - // Maintain previous behavior: strip the .worker suffix in the output filename - const workerBaseName = path.basename(args.path, ".worker"); - const workerOutputPath = `${workerBaseName}.js`; - - workerEntries.set(resolvedWorkerPath, workerOutputPath); - - // Return a virtual module that exports the worker URL - return { - path: args.path, - namespace: "worker-url", - }; - }); + build.onResolve({ filter: /\.worker(?:\.(?:m?ts|m?js|tsx))?$/ }, (args) => { + const isBare = !args.path.startsWith(".") && !path.isAbsolute(args.path); + const baseDir = isBare ? srcRoot : args.resolveDir || absWorkingDir; + + // Resolve to a concrete file, supporting .ts/.tsx/.mjs/.js, defaulting to .ts + const importPath = args.path; + const hasExt = /\.\w+$/.test(importPath); + const extCandidates = hasExt ? [""] : [".ts", ".tsx", ".mjs", ".js"]; + let resolvedWorkerPath; + for (const ext of extCandidates) { + const candidate = path.resolve(baseDir, `${importPath}${ext}`); + try { + // Avoid throwing if not found + if (require("node:fs").existsSync(candidate)) { + resolvedWorkerPath = candidate; + break; + } + } catch {} + } + if (!resolvedWorkerPath) { + resolvedWorkerPath = path.resolve(baseDir, `${importPath}.ts`); + } + + // Make output file unique and cache-busting: include relative dir and a short content hash + let relDir = path.relative(srcRoot, path.dirname(resolvedWorkerPath)); + if (relDir.startsWith("..")) { + // if outside srcRoot, fall back to absWorkingDir-based + relDir = path.relative(absWorkingDir, path.dirname(resolvedWorkerPath)); + } + const stem = path + .basename(importPath) + .replace(/\.worker(?:\.(?:m?ts|m?js|tsx))?$/, ""); + const relToken = relDir + .split(path.sep) + .filter(Boolean) + .join("_"); + const baseOut = `${relToken ? `${relToken}-` : ""}${stem}`; + const hash = require("node:crypto") + .createHash("sha256") + .update(require("node:fs").readFileSync(resolvedWorkerPath)) + .digest("hex") + .slice(0, 8); + const workerOutputPath = path.join("workers", `${baseOut}-${hash}.js`); + + workerEntries.set(resolvedWorkerPath, workerOutputPath); + + // Return a virtual module keyed by absolute path to avoid collisions + return { + path: resolvedWorkerPath, + namespace: "worker-url", + }; + });
🧹 Nitpick comments (5)
tools/esbuild/workerPlugin.js (5)
101-109: Reconsider forcing external: []Forcing external: [] will inline everything into worker bundles, even packages that are intentionally externalized in the main build (e.g., sentry loader, analytics). Unless you need full self-containment, prefer reusing main externals.
Apply this diff:
- external: [], + external: Array.isArray(initial.external) ? initial.external : [],If full self-containment is required, keep the current behavior.
4-12: Docs mismatch: imports end with “.worker” but description says “.worker.ts”Your onResolve filter targets .worker (and we also want to support .worker.ts/.worker.js). Tweak the docs to match the actual import style: import xxx from './foo.worker' and note that the source file may be .ts/.tsx/.js/.mjs.
Apply this diff to the commentary:
- * This plugin finds files ending with the `.worker.ts` suffix, bundles each into a + * This plugin finds import specifiers ending with `.worker` (optionally with an extension + * like `.worker.ts`), bundles each into a separate file, and provides the URL to the + * bundled worker script to the main application. @@ - * 1. `onResolve` intercepts imports for `.worker` files and registers them. + * 1. `onResolve` intercepts imports for `.worker[.<ext>]` files and registers them.Also applies to: 17-21
74-139: Stale worker file accumulation when using hashed filenamesIf you adopt hashed worker filenames, stale files will accumulate in outdir/workers across rebuilds. Consider a lightweight cleanup that removes previously-emitted worker files not present in activeWorkerPaths/workerEntries after a successful build. I can provide a follow-up patch if you want this automated.
118-126: Propagating worker build failures is good; consider surfacing the absolute pathNice propagation. Minor improvement: include the absolute worker path in the success log as well to ease debugging in large repos.
- console.log(`✓ Built worker: ${workerOutputPath}`); + console.log(`✓ Built worker: ${workerOutputPath} (${workerPath})`);
45-61: Optional: Support .worker. without assuming .ts*Even if the codebase currently uses .ts, adding extension probing makes the plugin more resilient and aligns with mixed TS/JS repos. The onResolve diff above already includes this enhancement.
Would you like me to extend it to prefer .ts over .js but respect an explicit extension if present?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
tools/esbuild/workerPlugin.js(1 hunks)
🔇 Additional comments (3)
tools/esbuild/workerPlugin.js (3)
28-39: Solid plugin structure and context reuseGood use of esbuild.context per-worker and caching across rebuilds. Resetting the registration map onStart and disposing unused contexts onEnd is a clean lifecycle.
35-37: Outdir/publicPath sanity checksIf initial.outdir is unset, the plugin defaults to absWorkingDir, which likely won’t be mapped by publicPath, yielding broken worker URLs. Add a guard and fail fast if outdir/publicPath are inconsistent.
Do we always run esbuild with outdir and a matching publicPath? If not, I can add a concise runtime check to throw a descriptive error.
45-66: No duplicate worker basenames found — no change requiredScanned the repo for *.worker.(ts|tsx|js|mjs) files: 10 found and no duplicate basenames. Stripping ".worker" for the output filenames does not cause collisions.
Files found:
- frontend/javascripts/libs/DRACOWorker.worker.js
- frontend/javascripts/viewer/workers/async_bucket_picker.worker.ts
- frontend/javascripts/viewer/workers/async_get_maximum_zoom_for_all_mags.worker.ts
- frontend/javascripts/viewer/workers/byte_array_lz4_compression.worker.ts
- frontend/javascripts/viewer/workers/byte_arrays_to_lz4_base64.worker.ts
- frontend/javascripts/viewer/workers/compress.worker.ts
- frontend/javascripts/viewer/workers/decode_four_bit.worker.ts
- frontend/javascripts/viewer/workers/fetch_buffer.worker.ts
- frontend/javascripts/viewer/workers/fetch_buffer_with_headers.worker.ts
- frontend/javascripts/viewer/workers/slow_byte_array_lz4_compression.worker.ts
| const esbuild = require("esbuild"); | ||
| const path = require("node:path"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing imports for fs/crypto used in filename hashing
If you adopt hashed filenames, you need fs and crypto.
Apply this diff:
const esbuild = require("esbuild");
const path = require("node:path");
+const fs = require("node:fs");
+const crypto = require("node:crypto");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const esbuild = require("esbuild"); | |
| const path = require("node:path"); | |
| const esbuild = require("esbuild"); | |
| const path = require("node:path"); | |
| const fs = require("node:fs"); | |
| const crypto = require("node:crypto"); |
🤖 Prompt for AI Agents
In tools/esbuild/workerPlugin.js around lines 23 to 25, the file uses fs and
crypto for filename hashing but does not import them; add the missing imports at
the top of the file (matching the existing style, e.g. const fs =
require('node:fs') and const crypto = require('node:crypto') or
require('fs')/require('crypto')) so the hashing and file operations work without
runtime errors.
| // Handle the virtual worker URL modules | ||
| build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => { | ||
| const workerBaseName = path.basename(args.path, ".worker"); | ||
| const workerUrl = `${publicPath}${workerBaseName}.js`; | ||
|
|
||
| return { | ||
| contents: `export default "${workerUrl}";`, | ||
| loader: "js", | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Derive the URL from the registered absolute key to match the unique outfile
onLoad is currently recomputing the URL from a basename, which will diverge once the output name includes directories and a hash. Read from workerEntries keyed by the absolute path returned in onResolve and emit a posix URL.
Apply this diff:
- build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => {
- const workerBaseName = path.basename(args.path, ".worker");
- const workerUrl = `${publicPath}${workerBaseName}.js`;
-
- return {
- contents: `export default "${workerUrl}";`,
- loader: "js",
- };
- });
+ build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => {
+ const workerOutputPath = workerEntries.get(args.path);
+ if (!workerOutputPath) {
+ return {
+ errors: [{ text: `wk-worker-bundler: unknown worker "${args.path}"` }],
+ };
+ }
+ const workerUrl = `${publicPath}${workerOutputPath.split(path.sep).join("/")}`;
+ return {
+ contents: `export default "${workerUrl}";`,
+ loader: "js",
+ };
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle the virtual worker URL modules | |
| build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => { | |
| const workerBaseName = path.basename(args.path, ".worker"); | |
| const workerUrl = `${publicPath}${workerBaseName}.js`; | |
| return { | |
| contents: `export default "${workerUrl}";`, | |
| loader: "js", | |
| }; | |
| }); | |
| // Handle the virtual worker URL modules | |
| build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => { | |
| const workerOutputPath = workerEntries.get(args.path); | |
| if (!workerOutputPath) { | |
| return { | |
| errors: [{ text: `wk-worker-bundler: unknown worker "${args.path}"` }], | |
| }; | |
| } | |
| const workerUrl = `${publicPath}${workerOutputPath.split(path.sep).join("/")}`; | |
| return { | |
| contents: `export default "${workerUrl}";`, | |
| loader: "js", | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In tools/esbuild/workerPlugin.js around lines 63 to 72, the plugin currently
builds the worker URL from path.basename(args.path, ".worker"), which will
mismatch when the emitted outfile includes directories or hashes; instead look
up the original absolute key in the plugin's workerEntries map (the same key
used in onResolve) using args.path, get the recorded emitted filename/output
path from that entry and produce a posix-style URL by joining publicPath with
that emitted filename (ensuring forward slashes), and return that string as the
exported default content so the URL matches the actual unique outfile.
| const workerPlugins = basePlugins.filter( | ||
| (p) => p && typeof p.name === "string" && !/worker/i.test(p.name), | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Overly-broad plugin exclusion heuristic
Filtering out plugins by /worker/i may unintentionally drop unrelated plugins whose names contain "worker". Instead, exclude only this plugin by exact name.
Apply this diff:
- const workerPlugins = basePlugins.filter(
- (p) => p && typeof p.name === "string" && !/worker/i.test(p.name),
- );
+ const workerPlugins = basePlugins.filter(
+ (p) => p && typeof p.name === "string" && p.name !== "wk-worker-bundler",
+ );Ensure any plugin that must run for workers (e.g., proto/WASM handling) remains included after this change.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show file (tools/esbuild/workerPlugin.js) ==="
sed -n '1,240p' tools/esbuild/workerPlugin.js || true
echo
echo "=== search for 'wk-worker-bundler' ==="
rg -n "wk-worker-bundler" || true
echo
echo "=== search for occurrences of 'worker' (case-insensitive) in tools/esbuild ==="
rg -n --hidden --ignore-case "worker" tools/esbuild || true
echo
echo "=== search for 'basePlugins' usage ==="
rg -n "basePlugins" || true
echo
echo "=== show plugin creation lines in tools (lines containing 'name:') ==="
rg -n "name:\s*['\"]" tools || trueLength of output: 12664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show file (tools/esbuild/protoPlugin.js) ==="
sed -n '1,240p' tools/esbuild/protoPlugin.js || true
echo
echo "=== search repo for 'wasm' (case-insensitive) ==="
rg -n --hidden --ignore-case "wasm" || true
echo
echo "=== search repo for 'protoPlugin' ==="
rg -n --hidden "protoPlugin" || true
echo
echo "=== search for 'plugins:' occurrences ==="
rg -n --hidden "plugins\s*:" || true
echo
echo "=== search for esbuild plugin patterns: 'setup(build)', 'onResolve(', 'onLoad(' ==="
rg -n --hidden "setup\(build\)" || true
rg -n --hidden "onResolve\(" || true
rg -n --hidden "onLoad\(" || trueLength of output: 9316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show file (esbuild_config.js) ==="
sed -n '1,240p' esbuild_config.js || true
echo
echo "=== show remainder if larger ==="
sed -n '241,480p' esbuild_config.js || trueLength of output: 5239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to fetch the plugin source from the web to confirm its name string if available via curl.
echo "=== curl raw plugin source ==="
curl -sL https://raw.githubusercontent.com/chialab/esbuild-plugin-worker/master/src/index.js | sed -n '1,240p' || true
echo
echo "=== curl package.json from repo ==="
curl -sL https://raw.githubusercontent.com/chialab/esbuild-plugin-worker/master/package.json | jq -r '.name, .version' || trueLength of output: 496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== registry info: @chialab/esbuild-plugin-worker latest ==="
curl -sS "https://registry.npmjs.org/@chialab/esbuild-plugin-worker/latest" | jq -r '.name, .version, .main' || true
echo
echo "=== try to fetch main file from unpkg for @chialab/esbuild-plugin-worker ==="
PKG_VER=$(curl -sS "https://registry.npmjs.org/@chialab/esbuild-plugin-worker/latest" | jq -r '.version' 2>/dev/null || true)
MAIN_FILE=$(curl -sS "https://registry.npmjs.org/@chialab/esbuild-plugin-worker/latest" | jq -r '.main' 2>/dev/null || true)
if [ -n "$PKG_VER" ] && [ -n "$MAIN_FILE" ]; then
echo "version: $PKG_VER, main: $MAIN_FILE"
echo "=== fetch unpkg content (first 240 lines) ==="
curl -sL "https://unpkg.com/@chialab/esbuild-plugin-worker@${PKG_VER}/${MAIN_FILE}" | sed -n '1,240p' || true
else
echo "Could not determine version/main for @chialab/esbuild-plugin-worker"
fi
echo
echo "=== registry info: esbuild-plugin-wasm latest ==="
curl -sS "https://registry.npmjs.org/esbuild-plugin-wasm/latest" | jq -r '.name, .version, .main' || true
echo
echo "=== try to fetch main file from unpkg for esbuild-plugin-wasm ==="
PKG_VER2=$(curl -sS "https://registry.npmjs.org/esbuild-plugin-wasm/latest" | jq -r '.version' 2>/dev/null || true)
MAIN_FILE2=$(curl -sS "https://registry.npmjs.org/esbuild-plugin-wasm/latest" | jq -r '.main' 2>/dev/null || true)
if [ -n "$PKG_VER2" ] && [ -n "$MAIN_FILE2" ]; then
echo "version: $PKG_VER2, main: $MAIN_FILE2"
echo "=== fetch unpkg content (first 240 lines) ==="
curl -sL "https://unpkg.com/esbuild-plugin-wasm@${PKG_VER2}/${MAIN_FILE2}" | sed -n '1,240p' || true
else
echo "Could not determine version/main for esbuild-plugin-wasm"
fiLength of output: 16518
Filter worker-related plugins by exact name — don't use a broad /worker/i test
The current regex excludes any plugin with "worker" in its name; that unintentionally couples unrelated plugins and is brittle. esbuild_config.js includes a third-party plugin whose name is exactly "worker" (from @chialab/esbuild-plugin-worker), so we should exclude known worker plugins by exact name instead of using /worker/i. Proto ("proto") and WASM ("wasm") plugins remain included under the change.
- File: tools/esbuild/workerPlugin.js — replace the broad filter.
Apply this diff:
- const workerPlugins = basePlugins.filter(
- (p) => p && typeof p.name === "string" && !/worker/i.test(p.name),
- );
+ const EXCLUDED_WORKER_PLUGINS = new Set(["wk-worker-bundler", "worker"]);
+ const workerPlugins = basePlugins.filter(
+ (p) => p && typeof p.name === "string" && !EXCLUDED_WORKER_PLUGINS.has(p.name),
+ );This keeps createProtoPlugin ("proto") and wasmLoader ("wasm") included and explicitly excludes the in-repo worker bundler and the external "worker" plugin to avoid build loops.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const workerPlugins = basePlugins.filter( | |
| (p) => p && typeof p.name === "string" && !/worker/i.test(p.name), | |
| ); | |
| const EXCLUDED_WORKER_PLUGINS = new Set(["wk-worker-bundler", "worker"]); | |
| const workerPlugins = basePlugins.filter( | |
| (p) => p && typeof p.name === "string" && !EXCLUDED_WORKER_PLUGINS.has(p.name), | |
| ); |
🤖 Prompt for AI Agents
In tools/esbuild/workerPlugin.js around lines 97 to 99, the filter currently
uses a broad /worker/i regex which accidentally excludes unrelated plugins;
replace that regex check with an explicit exclusion list (e.g., const
excludedNames = ['worker', 'worker-bundler'] or the exact in-repo worker plugin
name) and filter plugins by checking !excludedNames.includes(p.name) while still
allowing 'proto' and 'wasm' to pass; ensure the filter only excludes plugins
whose name exactly matches entries in the exclusion list.
There was a problem hiding this comment.
Awesome, thanks for taking the time to work on this.
I found one missing renaming and that the work urls might need cache busting. Please find my two comments below.
During testing I additionally noticed that the wasm files are cached by the browser as well. Thus, they might also need cache busting.
Moreover, somehow opening annotations on https://esbuildbundling.webknossos.xyz/ is really slow. WK tends to frees for multiple seconds before showing any data and this is definetly not normal. I can reproduce this reliably in firefox and chrome on linux.
Besides this wk still seems to work. Not sure what is causing the freezing here. A quick network tab inspection showed nothing.
| // Handle the virtual worker URL modules | ||
| build.onLoad({ filter: /.*/, namespace: "worker-url" }, (args) => { | ||
| const workerBaseName = path.basename(args.path, ".worker"); | ||
| const workerUrl = `${publicPath}${workerBaseName}.js`; |
There was a problem hiding this comment.
I would add a file content hash at the end of the url to act as a cache buster. Otherwise we might run into browser caching issues once we modify a worker.
I mean something like that (!pseudo code!):
const contentHash = hash(args.path.content);
const workerUrl = `${publicPath}${workerBaseName}.js?hash={contentHash}`;Alternatively, using the commit hash might work as done in main.scala.html. Although this might reload the workers more often.
| proxy.web(req, res, { target: `http://127.0.0.1:${PORT + 1}` }); | ||
| } | ||
|
|
||
| function toWebpackDev(req, res) { |
There was a problem hiding this comment.
Renaming is still needed here
| function toWebpackDev(req, res) { | |
| function toEsbuildDev(req, res) { |
|
Moreover, the AI in your post stats:
which might be worth a look. And
is still an open TODO. |
Ok, I just gave this another test as it seemed to happen on another branch as well and after a restart + browser cache clear I can no longer reliably reproduced this (chrome & firefox). Not sure what was going on... So maybe ignore this I'd say |
|
Unfortunately, |
|
So maybe we should open a bug report / issue at esbuild? Or close this PR? Maybe we'll get help from the esbuild people? |
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:
yarn installyarn startTODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)