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 a test polyfill, 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 Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
|
@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? |
…ld's output; requests are paused as long as builds are ongoing
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
esbuild_config.js (4)
95-97: Hardcoded paths may cause issues in different environmentsThe copy plugin pattern uses a hardcoded path that assumes the zip.js package structure remains constant.
Consider making this more robust:
copyPlugin({ patterns: [ { - from: "node_modules/@zip.js/zip.js/dist/z-worker.js", + from: require.resolve("@zip.js/zip.js/dist/z-worker.js"), to: path.join(buildOutDir, "z-worker.js"), }, ], }),
45-47: Use more conventional temp directory namingThe temporary directory name could be more descriptive and follow conventional patterns.
const buildOutDir = isWatch - ? path.join(os.tmpdir(), "webknossos-esbuild-dev") + ? path.join(os.tmpdir(), `webknossos-esbuild-dev-${process.pid}`) : outputPath;This would allow multiple dev instances to run simultaneously without conflicts.
147-147: Remove unnecessary/assets/images/*external entry
No static import or require of/assets/images/*was found, and esbuild’s external option only affects module specifiers—not string literals—so this entry has no effect. Remove it or revisit when you introduce actual image imports.
157-195: Consider using esbuild’s built-in serve for simple build-waiting
Esbuild’s native serve() already waits for in-progress builds and never serves stale output—if your only requirement is to hold requests until a rebuild finishes, you can replace this Express gateway with serve() to simplify. Retain this custom Express middleware only if you need request-level hooks (e.g., preflight OPTIONS, custom 503 gating, progress streaming, advanced proxying).
📜 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 (2)
esbuild_config.js(1 hunks)package.json(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
esbuild_config.js (2)
tools/esbuild/workerPlugin.js (3)
esbuild(23-23)path(24-24)createWorkerPlugin(28-140)tools/esbuild/protoPlugin.js (3)
path(5-5)fs(6-6)createProtoPlugin(10-57)
🔇 Additional comments (2)
package.json (2)
43-47: Remove compatibility suggestion for @chialab/esbuild-plugin-worker The plugin isn’t listed in your package.json, so no version mismatch needs addressing.Likely an incorrect or invalid review comment.
13-68: Webpack dependencies removed
package.json lists no webpack-related packages; only legacy comments reference webpack (e.g., in comlink_wrapper.ts/dependency-cruiser.js) and can be ignored or cleaned up as desired.
| const target = browserslistToEsbuild([ | ||
| "last 3 Chrome versions", | ||
| "last 3 Firefox versions", | ||
| "last 2 Edge versions", | ||
| "last 1 Safari versions", | ||
| "last 1 iOS versions", | ||
| ]); |
There was a problem hiding this comment.
Modern browser targets may break compatibility
The browser targets are set to very recent versions (last 3 Chrome/Firefox, last 2 Edge, last 1 Safari/iOS). This aggressive targeting will drop support for older browsers and could affect users on enterprise or locked-down systems.
Based on the PR objectives mentioning "drops legacy browser support" as an expected impact, this appears intentional. However, ensure this aligns with your user base requirements. Consider documenting the minimum supported browser versions explicitly in your documentation.
The current configuration effectively requires:
- Chrome 136+ (3 versions back from 139)
- Firefox 140+ (3 versions back from 143)
- Safari 18+ (1 version back)
- Module script and module worker support
If you need to support slightly older browsers, consider adjusting to:
const target = browserslistToEsbuild([
- "last 3 Chrome versions",
- "last 3 Firefox versions",
- "last 2 Edge versions",
- "last 1 Safari versions",
- "last 1 iOS versions",
+ "last 6 Chrome versions",
+ "last 6 Firefox versions",
+ "last 4 Edge versions",
+ "last 2 Safari versions",
+ "last 2 iOS versions",
]);📝 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 target = browserslistToEsbuild([ | |
| "last 3 Chrome versions", | |
| "last 3 Firefox versions", | |
| "last 2 Edge versions", | |
| "last 1 Safari versions", | |
| "last 1 iOS versions", | |
| ]); | |
| const target = browserslistToEsbuild([ | |
| "last 6 Chrome versions", | |
| "last 6 Firefox versions", | |
| "last 4 Edge versions", | |
| "last 2 Safari versions", | |
| "last 2 iOS versions", | |
| ]); |
🤖 Prompt for AI Agents
In esbuild_config.js around lines 26 to 32, the esbuild browser targets are set
very aggressively (last 3 Chrome/Firefox, last 2 Edge, last 1 Safari/iOS) which
will drop older/enterprise browsers; either relax the target list to include
older major versions you must support (e.g., increase the "last N" or add
explicit minimum versions for Chrome/Firefox/Safari/Edge/iOS) or add a separate
legacy build/targets map for older browsers and ensure module/script fallbacks,
and document the exact minimum supported browser versions in your repo docs (or
a comment next to this config) so the impact is explicit and reproducible.
| if (isWatch) { | ||
| // Ensure a clean, stable directory for development builds | ||
| fs.rmSync(buildOutDir, { recursive: true, force: true }); | ||
| fs.mkdirSync(buildOutDir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
Temporary directory cleanup in watch mode could cause issues
The code forcefully removes and recreates the temporary build directory on each watch mode start. This could cause issues if multiple instances are running or if the directory is in use.
if (isWatch) {
// Ensure a clean, stable directory for development builds
- fs.rmSync(buildOutDir, { recursive: true, force: true });
- fs.mkdirSync(buildOutDir, { recursive: true });
+ try {
+ fs.rmSync(buildOutDir, { recursive: true, force: true });
+ fs.mkdirSync(buildOutDir, { recursive: true });
+ } catch (error) {
+ console.error(`Failed to prepare build directory: ${error.message}`);
+ throw error;
+ }
}📝 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.
| if (isWatch) { | |
| // Ensure a clean, stable directory for development builds | |
| fs.rmSync(buildOutDir, { recursive: true, force: true }); | |
| fs.mkdirSync(buildOutDir, { recursive: true }); | |
| } | |
| if (isWatch) { | |
| // Ensure a clean, stable directory for development builds | |
| try { | |
| fs.rmSync(buildOutDir, { recursive: true, force: true }); | |
| fs.mkdirSync(buildOutDir, { recursive: true }); | |
| } catch (error) { | |
| console.error(`Failed to prepare build directory: ${error.message}`); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In esbuild_config.js around lines 51-55, avoid force-removing and recreating the
shared buildOutDir in watch mode; instead create a per-process temporary output
directory (e.g., use os.tmpdir()/fs.mkdtemp or append process.pid and a
timestamp to buildOutDir) and update references to use that dir, and if you must
remove an existing dir use non-force deletion with try/catch, locking/exists
checks, and exponential backoff retries to handle in-use directories rather than
fs.rmSync(..., { force: true }).
| let currentBuildCount = 0; | ||
| let buildStartTime = null; | ||
| let buildCounterPlugin = { | ||
| name: 'buildCounterPlugin', | ||
| setup(build) { | ||
| build.onStart(() => { | ||
| if (currentBuildCount === 0) { | ||
| buildStartTime = performance.now(); | ||
| } | ||
| currentBuildCount++; | ||
| console.log(now(), 'build started. currentBuildCount=', currentBuildCount) | ||
| }) | ||
| build.onEnd(() => { | ||
| currentBuildCount--; | ||
| if (currentBuildCount === 0) { | ||
| console.log("Build took", performance.now() - buildStartTime); | ||
| } | ||
| console.log(now(), 'build ended. currentBuildCount=', currentBuildCount) | ||
| if (currentBuildCount === 0) { | ||
| onBuildDone(); | ||
| } | ||
| }) | ||
| }, | ||
| } |
There was a problem hiding this comment.
Build tracking mechanism needs proper error handling
The build counter plugin tracks concurrent builds but doesn't handle errors that might leave the counter in an inconsistent state.
If a build fails catastrophically without triggering onEnd, the counter could remain elevated, causing requests to be permanently queued. Consider adding error handling:
let buildCounterPlugin = {
name: 'buildCounterPlugin',
setup(build) {
build.onStart(() => {
if (currentBuildCount === 0) {
buildStartTime = performance.now();
}
currentBuildCount++;
console.log(now(), 'build started. currentBuildCount=', currentBuildCount)
})
- build.onEnd(() => {
+ build.onEnd((result) => {
currentBuildCount--;
if (currentBuildCount === 0) {
console.log("Build took", performance.now() - buildStartTime);
}
console.log(now(), 'build ended. currentBuildCount=', currentBuildCount)
+ // Log errors if present
+ if (result.errors.length > 0) {
+ console.error(`Build completed with ${result.errors.length} errors`);
+ }
if (currentBuildCount === 0) {
onBuildDone();
}
})
+ // Ensure counter is decremented even on dispose
+ build.onDispose(() => {
+ if (currentBuildCount > 0) {
+ console.warn('Build context disposed with active builds, resetting counter');
+ currentBuildCount = 0;
+ onBuildDone();
+ }
+ })
},
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
esbuild_config.js lines 60-83: the build counter increments/decrements globally
and can remain elevated if a build fails without triggering onEnd; replace the
simple counter with a per-build tracking approach and add fail-safes: onStart
should generate and store a unique build id in a Map (or Set) and increment only
that entry; onEnd should remove that id and compute currentBuildCount from the
Map size so stray increments can't persist; additionally add a watchdog timeout
for each start (store a timer with the id) that will auto-clean the id and call
onBuildDone if onEnd never arrives, and register process-level handlers for
uncaughtException/unhandledRejection to clear active builds and call
onBuildDone; ensure all timers are cleared when builds finish.
| assetNames: "[name].[hash]", | ||
| sourcemap: isProduction ? "external" : "inline", |
There was a problem hiding this comment.
Missing cache busting for worker files
The PR objectives mention "cache-busting for workers (emit hashed filenames or append nocache)" as a follow-up item. The current configuration generates hashed chunk names but workers might not benefit from this.
The createWorkerPlugin in tools/esbuild/workerPlugin.js currently generates workers with static filenames (e.g., workerBaseName.js). This could cause caching issues when workers are updated.
Would you like me to help implement cache-busted filenames for workers or create an issue to track this task?
| function gateMiddleware(req, res, next) { | ||
| if (currentBuildCount === 0) { | ||
| return next(); // allow immediately | ||
| } | ||
|
|
||
| // hold request | ||
| waiting.push({ req, res, next }); | ||
| } |
There was a problem hiding this comment.
Request queueing mechanism could cause memory issues
The middleware queues all incoming requests when builds are in progress, which could lead to memory issues if many requests arrive during a long build.
Consider adding a queue size limit or timeout:
// queue for waiting requests
let waiting = [];
+const MAX_QUEUE_SIZE = 1000;
+const QUEUE_TIMEOUT_MS = 30000;
function gateMiddleware(req, res, next) {
if (currentBuildCount === 0) {
return next(); // allow immediately
}
+ if (waiting.length >= MAX_QUEUE_SIZE) {
+ res.status(503).send("Server too busy, please try again later");
+ return;
+ }
+
+ const timeout = setTimeout(() => {
+ const index = waiting.findIndex(w => w.req === req);
+ if (index !== -1) {
+ waiting.splice(index, 1);
+ res.status(504).send("Build timeout");
+ }
+ }, QUEUE_TIMEOUT_MS);
+
// hold request
- waiting.push({ req, res, next });
+ waiting.push({ req, res, next, timeout });
}📝 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.
| function gateMiddleware(req, res, next) { | |
| if (currentBuildCount === 0) { | |
| return next(); // allow immediately | |
| } | |
| // hold request | |
| waiting.push({ req, res, next }); | |
| } | |
| // queue for waiting requests | |
| let waiting = []; | |
| const MAX_QUEUE_SIZE = 1000; | |
| const QUEUE_TIMEOUT_MS = 30000; | |
| function gateMiddleware(req, res, next) { | |
| if (currentBuildCount === 0) { | |
| return next(); // allow immediately | |
| } | |
| if (waiting.length >= MAX_QUEUE_SIZE) { | |
| res.status(503).send("Server too busy, please try again later"); | |
| return; | |
| } | |
| const timeout = setTimeout(() => { | |
| const index = waiting.findIndex(w => w.req === req); | |
| if (index !== -1) { | |
| waiting.splice(index, 1); | |
| res.status(504).send("Build timeout"); | |
| } | |
| }, QUEUE_TIMEOUT_MS); | |
| // hold request | |
| waiting.push({ req, res, next, timeout }); | |
| } |
🤖 Prompt for AI Agents
In esbuild_config.js around lines 165 to 172, the gateMiddleware currently
pushes every incoming request into waiting while a build runs, which can cause
unbounded memory growth; add a bounded queue and timeout: define a
MAX_QUEUE_LENGTH and REQUEST_TIMEOUT_MS constant, and when pushing a request
check queue length and immediately respond with a 503 (or next an error) if the
queue is full; when enqueuing set a timer that will remove the request from the
waiting array and respond with 503 if it exceeds REQUEST_TIMEOUT_MS; ensure
timers are cleared when requests are later drained after the build completes so
no leaks occur, and log queue overflow/timeouts for observability.
| process.on("SIGINT", async () => { | ||
| await ctx.dispose(); | ||
| process.exit(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
SIGINT handler should handle cleanup errors
The signal handler for graceful shutdown doesn't handle potential errors during context disposal.
process.on("SIGINT", async () => {
+ console.log("\nShutting down development server...");
+ try {
await ctx.dispose();
+ console.log("Build context disposed successfully");
+ } catch (error) {
+ console.error("Error disposing build context:", error);
+ }
process.exit(0);
});📝 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.
| process.on("SIGINT", async () => { | |
| await ctx.dispose(); | |
| process.exit(0); | |
| }); | |
| process.on("SIGINT", async () => { | |
| console.log("\nShutting down development server..."); | |
| try { | |
| await ctx.dispose(); | |
| console.log("Build context disposed successfully"); | |
| } catch (error) { | |
| console.error("Error disposing build context:", error); | |
| } | |
| process.exit(0); | |
| }); |
🤖 Prompt for AI Agents
In esbuild_config.js around lines 191 to 194, the SIGINT handler calls await
ctx.dispose() without error handling; wrap the dispose call in a try/catch, log
or report any error thrown during ctx.dispose() (using console.error or the
project logger), and then call process.exit(0) on success or process.exit(1) on
failure so the process exits even when cleanup fails.
| "@types/color-hash": "^1.0.2", | ||
| "@types/cwise": "^1.0.4", | ||
| "@types/dagre": "^0.7.48", | ||
| "@types/express": "^5", |
There was a problem hiding this comment.
Express v5 in production dependencies
The type definitions for Express are included (@types/express), but the actual Express runtime dependency is placed in devDependencies. Since the esbuild dev server uses Express at runtime, consider whether this should be a regular dependency if the dev server is used in any non-local development scenarios.
If the Express server in esbuild_config.js is only used locally during development, the current placement is correct. However, if there's any scenario where the build tooling runs in CI or other environments that might not install devDependencies, this could cause issues.
🤖 Prompt for AI Agents
package.json around line 19: the review flags that @types/express is present but
the actual express runtime is in devDependencies which will break environments
that don't install devDependencies (CI or any non-local use of the esbuild dev
server); verify whether the esbuild dev server in esbuild_config.js is ever run
outside local dev—if it can run in CI/non-local, move the "express" package from
devDependencies to dependencies (keep @types/express in devDependencies or
devDependencies-only type entries), update package.json accordingly and
regenerate the lockfile to ensure consistent installs across environments.
|
see https://scm.slack.com/archives/C5AKLAV0B/p1758889098234979 for an update regarding the the "serve" problems. tldr: fixed now, but incremental building is slow (~3.5s on my machine which is worse than webpack). |
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)