Conversation
Coverage Report for CI Build 34904434328Coverage increased (+0.1%) to 95.007%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate implementation issues and one test-typing nit remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds independent esbuild bundle roots, isolating code-splitting graphs while supporting production builds, watch mode, validation, reporting, and documentation.
Changes:
- Partitions entries by deepest matching bundle root.
- Adds grouped build orchestration, metadata aggregation, cleanup, and settings reloads.
- Adds comprehensive tests and configuration documentation.
File summaries
| File | Summary | Review findings |
|---|---|---|
lib/build-esbuild/index.js |
Implements bundle-root partitioning and grouped builds. | Moderate: detect collisions in outputFiles when metafiles are disabled (2 votes). Moderate: serialize multiple build failures without a TypeError (2 votes). Moderate: prevent unbounded settings-module/cache growth during watch reloads (2 votes). |
lib/build-esbuild/bundle-roots.test.js |
Tests validation, isolation, watch behavior, and reloads. | Nit: use the file-level @import block and PluginBuild annotations in plugin tests (2 votes). |
docs/settings/README.md |
Documents bundle-root configuration and limitations. | No findings. |
Review details
Suppressed comments (2)
lib/build-esbuild/bundle-roots.test.js:313
- Please use the file-level
@importblock forPluginBuildand annotate this parameter asPluginBuildinstead of using an inlineimport('esbuild')type, matching the repository's JSDoc type-import convention.
setup (/** @type {import('esbuild').PluginBuild} */ build) {
lib/build-esbuild/bundle-roots.test.js:351
- Please use the file-level
@importblock forPluginBuildand annotate this parameter asPluginBuildinstead of using an inlineimport('esbuild')type, matching the repository's JSDoc type-import convention.
setup (/** @type {import('esbuild').PluginBuild} */ build) {
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const hash = createHash('sha256').update(await readFile(filepath)).digest('hex') | ||
| let contentUrls = settingsContentUrls.get(url.href) | ||
| if (!contentUrls) { | ||
| contentUrls = new Map() | ||
| settingsContentUrls.set(url.href, contentUrls) | ||
| } | ||
| let contentUrl = contentUrls.get(hash) | ||
| if (!contentUrl) { | ||
| // First load must share state with callers importing the ordinary file URL. | ||
| // Repeated/reverted contents reuse their original identity, not a new module. | ||
| if (contentUrls.size > 0) { | ||
| url.searchParams.set('domstack', hash) | ||
| // A new ESM URL alone does not invalidate Node's underlying CommonJS cache. | ||
| delete require.cache[resolve(filepath)] | ||
| } | ||
| contentUrl = url.href | ||
| contentUrls.set(hash, contentUrl) |
There was a problem hiding this comment.
Addressed in d100933. Node does not expose ESM module eviction, so removing entries from our map would not reclaim those modules. The extracted settings loader now enforces a process-wide budget of 256 distinct module versions across all settings paths, counting failed imports too. Cached and reverted contents reuse existing identities; new contents beyond the budget fail with an actionable process-restart message. This bounds DOMStack-created identities rather than claiming bounded total memory. Added limit/reuse/failure regression coverage and documented the tradeoff; imported dependencies still need a process restart. Full Node suite, project lint, and TypeScript pass. Leaving this thread unresolved for review.
There was a problem hiding this comment.
Following discussion with the maintainer, superseding the reload-budget solution: removed the hash-based settings loader, version tracking, and 256-version limit entirely. Settings now use an ordinary file-URL import, preserving the pre-feature module caching behavior. Changes to settings (including bundleRoots) or their dependencies require a DOMStack process restart; restarting esbuild contexts alone is not sufficient. Documented this explicitly and adjusted ESM/CommonJS identity tests. Proper worker-isolated reloads are outside this PR. Full Node suite, project lint, and TypeScript checks pass. Leaving the thread unresolved as requested.
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate issues remain in lib/build-esbuild/index.js: incomplete glob rejection, root-escaping name templates, and double error serialization.
Review details
Suppressed comments (3)
lib/build-esbuild/index.js:346
- The glob guard only looks for
*, so other glob patterns such as?, character classes, or brace expansions are still treated as literal entry paths and partitioned incorrectly instead of being rejected as promised by the bundle-roots contract. Use a complete glob check (or expand globs before partitioning) so every supported glob form is rejected when roots are configured.
if (entryInputs(entryPoints).some(input => input.includes('*'))) {
throw new TypeError('bundleRoots does not support glob entryPoints. Use explicit file paths instead.')
lib/build-esbuild/index.js:445
- Prefixing a user-supplied
chunkNamesorassetNamestemplate does not keep it beneath the named root when the template contains..; for example,assetNames: '../assets/[name]'becomesadmin/../assets/[name]and escapesadmin, allowing collisions with other graphs and violating the documented isolation guarantee. Normalize and reject templates that resolve outside the bundle root before passing them to esbuild.
return `${bundleRoot}/${template.replaceAll('\\', '/').replace(/^\/+/, '')}`
lib/build-esbuild/index.js:519
- These rejection reasons are serialized here and then passed through
serializeEsbuildErroragain in the catch block below. When an esbuild diagnostic has anErrorindetail, the first pass turns it into a plain{ name, message, stack }object and the second pass converts that object to"[object Object]", losing the structured detail metadata. Keep the raw rejection reasons here and let the outer catch serialize the single error or aggregate exactly once.
const failures = settled.filter(result => result.status === 'rejected').map(result => serializeEsbuildError(result.reason))
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
a374c76 to
4e145c8
Compare
Closes #261.
Summary
Allow sites to isolate sections such as admin applications into independent esbuild graphs, preventing their entry points from participating in code splitting with the public site.
Configure the bundleRoots named export in esbuild.settings.js or its supported variants with source-relative directories such as admin and account/internal.
Behavior
The settings transform runs once before partitioning; plugins are set up for each resulting build/context. Dependencies imported across roots are bundled independently by design, trading some duplication for isolation. Bundle roots do not enforce import boundaries or access permissions.
Compatibility and limitations
Sites without configured roots retain single-graph behavior. Explicit entry outputs take precedence over cross-root dynamic-import copies when generating page asset references. Production failures wait for all started builds to finish before returning.
Independent mangle caches remain available in per-group reports rather than being merged. Settings use ordinary module imports, matching the pre-feature behavior. Changes to settings, including bundleRoots, or their imported dependencies require a DOMStack process restart; restarting watch contexts alone does not reload them. Output collision checks are not transactional, so failed builds can leave partial output.
Testing