fix(bundler): port split/01 review patches#5906
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a typed global bundle-store declaration and a type-only import; tightens ManifestStore bundle validation and lookup; converts ManifestLoader and related manifest I/O to async with optional subprocess generation; simplifies bundler output to a single worker entry; and reworks externals resolution to include optionalDependencies and memoization. Changes
Sequence DiagramsequenceDiagram
participant App as App/Caller
participant ML as ManifestLoader.load()
participant Store as ManifestStore
participant FS as Filesystem
participant Gen as generate-manifest (subprocess)
App->>ML: load(baseDir, options)
ML->>Store: check globalThis bundle store
alt bundled store exists && baseDir matches
Store-->>ML: bundled ManifestStore
ML-->>App: return store
else manifest file exists on disk
ML->>FS: async read manifest.json
FS-->>ML: manifest data
ML->>Store: Store.fromBundle(data) (validate/init)
Store-->>ML: ManifestStore
ML-->>App: return store
else missing && autoGenerate enabled
ML->>Gen: exec via execaNode (stdin: JSON payload)
Gen->>Gen: resolve framework entry via package.json exports
Gen->>FS: write generated manifest.json
FS-->>ML: generated manifest data
ML->>Store: fromBundle(data)
ML-->>App: return store
else missing && autoGenerate disabled
ML-->>App: throw not found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5906 +/- ##
==========================================
- Coverage 85.04% 85.03% -0.01%
==========================================
Files 665 665
Lines 19100 19104 +4
Branches 3716 3717 +1
==========================================
+ Hits 16244 16246 +2
- Misses 2463 2465 +2
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
db10cc8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3755f13c.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-egg-40-split-0.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
db10cc8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://56c734ae.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-egg-40-split-0.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the manifest loading and bundling process across the core and bundler packages. Key changes include the introduction of a global bundle store with base directory matching in @eggjs/core, and the removal of the redundant agent entry in EntryGenerator for single-mode applications. The ExternalsResolver now features caching and improved ESM detection, while the ManifestLoader has been refactored to use asynchronous file operations, execaNode for manifest generation, and more robust path normalization for nested dependencies. Feedback was provided regarding the exports resolution logic to ensure it strictly adheres to Node.js shorthand condition rules and the need for more precise path segment matching in ManifestLoader to avoid over-broad string checks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/egg-bundler/src/scripts/generate-manifest.mjs (1)
44-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce manifest invalidation before generation to avoid stale bundle metadata.
This flow starts the app and writes a new manifest without first invalidating any prior
.egg/manifest.json. If an old manifest still passes invalidation checks, generation can short-circuit and miss newly added modules/files.💡 Suggested fix
const { ManifestStore } = await import('@eggjs/core'); + ManifestStore.clean(options.baseDir); let framework; if (options.frameworkEntry) {Based on learnings: in
tegg/plugin/config/src/app.ts, “to force a fresh scan ... callers must invokeManifestStore.clean()first to invalidate the cached manifest.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/scripts/generate-manifest.mjs` around lines 44 - 53, The manifest is being generated without first invalidating any cached manifest, which can cause stale metadata; call ManifestStore.clean(options.baseDir) before invoking app.loader.generateManifest() so the loader performs a fresh scan, then proceed to generate the manifest with app.loader.generateManifest() and write it with ManifestStore.write(options.baseDir, manifest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 146-149: The current try/catch around fsp.readFile in
ManifestLoader (the code using this.#manifestPath and fsp.readFile) silences all
errors; change the catch to inspect the thrown error (e.g., treat it as an
ErrnoException) and only return undefined when err.code === 'ENOENT', otherwise
rethrow or throw the original error so permission, EISDIR, and other I/O errors
surface instead of being treated as "manifest not found".
- Around line 418-425: The dep collection misses optionalDependencies, causing
normalizeRelKey to misrewrite hoisted optional packages; update the pkg type to
include optionalDependencies?: Record<string,string> and include
Object.keys(pkg.optionalDependencies ?? {}) when building depNames (the array
currently built from pkg.dependencies and pkg.devDependencies) so the
normalization graph used by createRequire/packageJsonPath and normalizeRelKey
includes optional packages as well.
---
Outside diff comments:
In `@tools/egg-bundler/src/scripts/generate-manifest.mjs`:
- Around line 44-53: The manifest is being generated without first invalidating
any cached manifest, which can cause stale metadata; call
ManifestStore.clean(options.baseDir) before invoking
app.loader.generateManifest() so the loader performs a fresh scan, then proceed
to generate the manifest with app.loader.generateManifest() and write it with
ManifestStore.write(options.baseDir, manifest).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2e69dbd-ebf8-459f-bc3c-86bf598bb4dc
⛔ Files ignored due to path filters (3)
tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/@swc/helpers/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/egg/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/esm-string-export/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (14)
packages/core/src/global.d.tspackages/core/src/index.tspackages/core/src/loader/manifest.tspackages/core/test/loader/manifest.test.tstools/egg-bundler/package.jsontools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/src/scripts/generate-manifest.mjstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/ManifestLoader.test.tstools/egg-bundler/test/fixtures/externals/basic-app/package.jsontools/egg-bundler/test/integration.test.ts
💤 Files with no reviewable changes (1)
- tools/egg-bundler/src/lib/EntryGenerator.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Ports a set of bundler/core review-fix patches onto next, focusing on bundled manifest loading/generation, externals resolution rules, and simplifying bundle entry generation for single-mode.
Changes:
- Update
ManifestLoaderto supportManifestStore.fromBundle, improve manifest JSON error reporting, and revise normalization/auto-generation behavior. - Refine
ExternalsResolverrules (optionalDependencies, ESM-only detection with stringexports, caching) and update fixtures/tests accordingly. - Generate only a worker entry by default (no agent entry in single-mode) and adjust bundler integration tests/manifest expectations.
Reviewed changes
Copilot reviewed 13 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/test/integration.test.ts | Updates integration expectations for bundled framework packages and externals list. |
| tools/egg-bundler/test/fixtures/externals/basic-app/package.json | Expands fixture dependencies to cover new externals resolver cases (optional deps, helpers, egg). |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/esm-string-export/package.json | Adds pure-ESM fixture with string exports to validate ESM-only detection. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/egg/package.json | Adds egg fixture package to validate “framework bundled by default” behavior. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/@swc/helpers/package.json | Adds helpers fixture to validate “helper bundled by default” behavior. |
| tools/egg-bundler/test/ManifestLoader.test.ts | Adds coverage for manifest normalization, default autoGenerate behavior, JSON parse errors, and exports resolution. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Adds coverage for optionalDependencies, string exports ESM-only, malformed package.json errors, and override semantics. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Updates expectations to worker-only entry generation; asserts agent entry absence. |
| tools/egg-bundler/src/scripts/generate-manifest.mjs | Makes option input robust (argv vs stdin), fixes absolute framework import, and removes forced exit. |
| tools/egg-bundler/src/lib/ManifestLoader.ts | Switches to async fs, uses ManifestStore.fromBundle, execa-based subprocess, and new normalization/module-map logic. |
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Adds optionalDependencies handling, caching, string-exports ESM-only detection, and stricter package.json read errors. |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Removes agent entry generation and related API surface. |
| tools/egg-bundler/package.json | Adjusts exports shape, adds execa dependency, adds @typescript/native-preview dev dependency. |
| packages/core/test/loader/manifest.test.ts | Adds tests for bundle store hook + fromBundle behavior and cleans up global state after tests. |
| packages/core/src/loader/manifest.ts | Adds typed global bundle store key, baseDir matching for bundle store, and hardens fromBundle validation. |
| packages/core/src/index.ts | Pulls in new global typing module. |
| packages/core/src/global.d.ts | Adds global typing for __EGG_BUNDLE_STORE__. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 334-343: The normalization loop must avoid overwriting earlier
entries when different source keys collapse to the same normalized key: in the
fileDiscovery loop (data.fileDiscovery → normalizedDiscovery) detect if
normalizedDiscovery[newKey] already exists and, instead of assigning, merge the
arrays by appending only new/unique file paths to preserve all discovered files;
in the resolveCache loop (data.resolveCache → normalizedResolveCache) only set
normalizedResolveCache[newKey] if the key is not already present—if it is
present and the existing value differs from newValue, do not overwrite (keep the
first value) or record the conflict via an existing logger mechanism; use the
existing `#normalizeRelKey` helper to compute newKey/newValue.
- Around line 425-435: The catch in addPackageDeps is swallowing read/parse
errors for a resolved package.json (pkg JSON.parse(await
fsp.readFile(packageJsonPath, 'utf-8'))), truncating the normalization graph;
change the handler to fail fast by rethrowing the original error (or throw a new
error with context including packageJsonPath and parentNormalizedDir) instead of
silently returning so malformed JSON or I/O/permission errors surface
immediately.
- Around line 358-365: The fast-path in ManifestLoader (around
this.#pathSegments(relKey) and the if that checks !path.isAbsolute(relKey) &&
!relKey.startsWith('..') && !segments.includes('node_modules') &&
!segments.includes('.pnpm')) uses the raw relKey so inputs like
"foo/../../outside" or paths that only reveal node_modules/.pnpm after collapse
slip through; normalize/collapse relKey first (e.g., call path.normalize or
equivalent and strip any leading './' when present), recompute segments from the
normalized value, and then run the existing checks (path.isAbsolute,
startsWith('..'), segments.includes(...)) so escaping and hidden
node_modules/.pnpm are detected correctly before taking the fast-path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 404c0ddf-f5e3-4209-9b87-e86a294afe01
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/ManifestLoader.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bundler/test/ManifestLoader.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tools/egg-bundler/src/lib/ManifestLoader.ts (3)
425-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hide parse or I/O failures for resolved
package.jsonfiles.At this point
packageJsonPathis either the app/framework package or a dependency thatrequire.resolve()already found. Swallowing read/parse errors silently truncates the module map and can mis-normalize manifest paths instead of failing the broken install early.Suggested fix
let pkg: { dependencies?: Record<string, string>; optionalDependencies?: Record<string, string>; devDependencies?: Record<string, string>; }; try { pkg = JSON.parse(await fsp.readFile(packageJsonPath, 'utf-8')); - } catch { - return; + } catch (error) { + throw new Error(`[`@eggjs/egg-bundler`] failed to read ${packageJsonPath}`, { cause: error }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ManifestLoader.ts` around lines 425 - 435, The catch block in addPackageDeps currently swallows read/parse errors for packageJsonPath (read via fsp.readFile and parsed with JSON.parse), which hides broken installs; update the catch to surface the error instead of returning silently—either rethrow the caught error or wrap it with a descriptive message and throw (including packageJsonPath), so failures reading/parsing package.json inside addPackageDeps are not ignored and will fail fast.
333-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge or reject entries when normalization collapses keys.
Different source paths can normalize onto the same stable key here. Plain assignment drops earlier
fileDiscoveryentries and silently changes the winner inresolveCache, which can lose discovered files or mask conflicting cache values.Suggested fix
const normalizedDiscovery: Record<string, string[]> = {}; for (const [key, files] of Object.entries(data.fileDiscovery)) { const newKey = await this.#normalizeRelKey(key, moduleMap); - normalizedDiscovery[newKey] = files; + const existing = normalizedDiscovery[newKey]; + normalizedDiscovery[newKey] = existing + ? Array.from(new Set([ ...existing, ...files ])) + : [ ...files ]; } const normalizedResolveCache: Record<string, string | null> = {}; for (const [key, value] of Object.entries(data.resolveCache)) { const newKey = await this.#normalizeRelKey(key, moduleMap); const newValue = value === null ? null : await this.#normalizeRelKey(value, moduleMap); - normalizedResolveCache[newKey] = newValue; + if (Object.hasOwn(normalizedResolveCache, newKey) && normalizedResolveCache[newKey] !== newValue) { + throw new Error(`[`@eggjs/egg-bundler`] conflicting normalized resolveCache entry for ${newKey}`); + } + normalizedResolveCache[newKey] = newValue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ManifestLoader.ts` around lines 333 - 343, Normalization can collapse multiple source keys into the same stable key, but the loops in ManifestLoader that build normalizedDiscovery and normalizedResolveCache currently overwrite earlier entries; update the logic in the code that iterates over data.fileDiscovery and data.resolveCache (the places calling this.#normalizeRelKey) to detect when newKey already exists: for normalizedDiscovery, merge the incoming files array into the existing array and deduplicate entries instead of replacing; for normalizedResolveCache, detect conflicting non-null values for the same newKey and handle deterministically (either throw/log a warning and keep the first value, or reconcile according to project policy) rather than silently overwriting—ensure you reference the methods/variables normalizedDiscovery, normalizedResolveCache, fileDiscovery, resolveCache and `#normalizeRelKey` when making the change.
356-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollapse
relKeybefore taking the in-base fast path.
foo/../../outsidestill passes the current guard because the raw string neither starts with..nor containsnode_modules/.pnpm. That leaves escaping paths unnormalized and install-layout-dependent.Suggested fix
async `#normalizeRelKey`(relKey: string, moduleMap: ModuleMapEntry[]): Promise<string> { if (!relKey) return relKey; - const segments = this.#pathSegments(relKey); + const abs = path.resolve(this.#baseDir, relKey); + const relativeToBase = path.relative(this.#baseDir, abs).replaceAll(path.sep, '/'); + const segments = this.#pathSegments(relativeToBase); // Already inside baseDir, relative, and not escaping — leave as-is. if ( !path.isAbsolute(relKey) && - !relKey.startsWith('..') && + !relativeToBase.startsWith('..') && + !path.isAbsolute(relativeToBase) && !segments.includes('node_modules') && !segments.includes('.pnpm') ) { return relKey; } - const abs = path.resolve(this.#baseDir, relKey); const realAbs = await this.#realpath(abs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ManifestLoader.ts` around lines 356 - 369, In `#normalizeRelKey`, collapse/normalize relKey before the "in-base fast path" check so escaping segments like "foo/../../outside" are caught; specifically compute a normalized form (e.g. const normRel = path.normalize(relKey) or an equivalent collapse) and use normRel with this.#pathSegments(normRel), path.isAbsolute(normRel), and normRel.startsWith('..') in the guard, and return normRel when taking the fast path; leave the subsequent resolution logic (using this.#baseDir, path.resolve and this.#realpath) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 425-435: The catch block in addPackageDeps currently swallows
read/parse errors for packageJsonPath (read via fsp.readFile and parsed with
JSON.parse), which hides broken installs; update the catch to surface the error
instead of returning silently—either rethrow the caught error or wrap it with a
descriptive message and throw (including packageJsonPath), so failures
reading/parsing package.json inside addPackageDeps are not ignored and will fail
fast.
- Around line 333-343: Normalization can collapse multiple source keys into the
same stable key, but the loops in ManifestLoader that build normalizedDiscovery
and normalizedResolveCache currently overwrite earlier entries; update the logic
in the code that iterates over data.fileDiscovery and data.resolveCache (the
places calling this.#normalizeRelKey) to detect when newKey already exists: for
normalizedDiscovery, merge the incoming files array into the existing array and
deduplicate entries instead of replacing; for normalizedResolveCache, detect
conflicting non-null values for the same newKey and handle deterministically
(either throw/log a warning and keep the first value, or reconcile according to
project policy) rather than silently overwriting—ensure you reference the
methods/variables normalizedDiscovery, normalizedResolveCache, fileDiscovery,
resolveCache and `#normalizeRelKey` when making the change.
- Around line 356-369: In `#normalizeRelKey`, collapse/normalize relKey before the
"in-base fast path" check so escaping segments like "foo/../../outside" are
caught; specifically compute a normalized form (e.g. const normRel =
path.normalize(relKey) or an equivalent collapse) and use normRel with
this.#pathSegments(normRel), path.isAbsolute(normRel), and
normRel.startsWith('..') in the guard, and return normRel when taking the fast
path; leave the subsequent resolution logic (using this.#baseDir, path.resolve
and this.#realpath) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72930a57-f307-4d68-b312-ea1f7937320e
📒 Files selected for processing (4)
packages/core/src/global.tspackages/core/src/index.tstools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/ManifestLoader.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/index.ts
- packages/core/src/global.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Ports a set of review-fix deltas from the split branch onto next, focusing on bundled startup-manifest support in @eggjs/core and reliability/behavior changes in @eggjs/egg-bundler (manifest loading/generation, externals resolution, and entry generation).
Changes:
- Add bundled-manifest store support in
@eggjs/core(global bundle store registration +ManifestStore.fromBundle()hardening + typing). - Update bundler behavior: framework packages are bundled by default, externals resolution considers optionalDependencies, and agent entry generation is removed (single worker entry only).
- Harden/expand manifest loading and generation (async IO, normalization across dependency layouts, stricter errors, and auto-generation disabled by default).
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/test/integration.test.ts | Updates integration expectations for framework packages now being bundled (not externalized). |
| tools/egg-bundler/test/fixtures/externals/basic-app/package.json | Expands fixture dependencies (egg/@swc/helpers/optional deps) to validate new externals rules. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/esm-string-export/package.json | Adds ESM-only fixture using string exports for resolver coverage. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/egg/package.json | Adds egg fixture package metadata for resolver tests. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/@swc/helpers/package.json | Adds @swc/helpers fixture package metadata for resolver tests. |
| tools/egg-bundler/test/ManifestLoader.test.ts | Adds comprehensive tests for manifest normalization, error surfacing, and framework entry resolution. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Updates/extends resolver tests for optionalDependencies, framework bundling defaults, and ESM-only detection. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Updates tests to reflect worker-only entry generation (no agent entry). |
| tools/egg-bundler/src/scripts/generate-manifest.mjs | Improves options ingestion (argv or stdin) and framework import behavior. |
| tools/egg-bundler/src/lib/ManifestLoader.ts | Makes manifest IO async, disables auto-generation by default, hardens JSON/version errors, and expands normalization/entry resolution. |
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Revises externals selection (optionalDependencies, caching, ESM-only detection changes, framework no longer auto-externalized). |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Removes agent entry output and keeps worker entry as the sole generated entry. |
| tools/egg-bundler/package.json | Makes exports/publishConfig.exports explicit for types + import, and adds execa dependency. |
| packages/core/test/loader/manifest.test.ts | Adds coverage for bundle store behavior and fromBundle() validation/hardening. |
| packages/core/src/loader/manifest.ts | Adds bundle store registration keyed on baseDir, introduces fromBundle() validation, and uses a typed global key. |
| packages/core/src/index.ts | Ensures global augmentation module is included in type output via import type {}. |
| packages/core/src/global.ts | Adds typed globalThis.__EGG_BUNDLE_STORE__ augmentation. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/egg-bundler/src/lib/ManifestLoader.ts (1)
317-324:⚠️ Potential issue | 🟠 MajorHandle
ERR_PACKAGE_PATH_NOT_EXPORTEDin#resolveFrameworkPath()instead of silently treating the framework specifier as a path.When
require.resolve("${framework}/package.json")fails because./package.jsonis not exported in the package's "exports" field, the catch block returns the bare package name. This causes the subsequentpath.join(frameworkDir, 'package.json')to resolve relative to the current working directory rather than the installed dependency.The
resolvePackageJson()function defined in the same file already handles this case correctly: it catchesERR_PACKAGE_PATH_NOT_EXPORTEDspecifically and uses fallback logic (viafindPackageJsonFromNodeModules()or walking up from the main entry point) to locate the package.json. Apply the same pattern here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ManifestLoader.ts` around lines 317 - 324, The catch in `#resolveFrameworkPath`() silently returns the package name when require.resolve(`${this.#framework}/package.json`) fails, which breaks resolution for packages that hide package.json via "exports"; change it to specifically handle the ERR_PACKAGE_PATH_NOT_EXPORTED error by delegating to the existing resolvePackageJson(this.#framework) fallback (which uses findPackageJsonFromNodeModules() and other walk-up logic) and return the dirname of that resolved package.json, while rethrowing any other unexpected errors; ensure you still return this.#framework if resolvePackageJson() can’t find a package.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 259-274: The code currently prioritizes PACKAGE_ENTRY_CONDITIONS
before honoring the object's declared key order; change ManifestLoader's
package-export resolution to iterate Object.entries(map) in insertion order and,
for each [key, value], skip keys starting with '.' and skip any condition keys
that are not in the active condition set (so they are not treated as fallbacks),
then call this.#resolvePackageEntry(value) and return the first resolved result;
remove the pre-check loop that used PACKAGE_ENTRY_CONDITIONS and the separate
"used" logic so resolution follows the package.json exports object key order
(affecting frameworkEntry resolution).
---
Outside diff comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 317-324: The catch in `#resolveFrameworkPath`() silently returns the
package name when require.resolve(`${this.#framework}/package.json`) fails,
which breaks resolution for packages that hide package.json via "exports";
change it to specifically handle the ERR_PACKAGE_PATH_NOT_EXPORTED error by
delegating to the existing resolvePackageJson(this.#framework) fallback (which
uses findPackageJsonFromNodeModules() and other walk-up logic) and return the
dirname of that resolved package.json, while rethrowing any other unexpected
errors; ensure you still return this.#framework if resolvePackageJson() can’t
find a package.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c41b634-fa95-4e29-ac8d-79453c605c15
📒 Files selected for processing (4)
tools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/ManifestLoader.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bundler/test/ExternalsResolver.test.ts
Contributor
Author
|
Follow-up pushed in 6a38cd4 for the latest CodeRabbit feedback: export condition objects now resolve in package key order using active import conditions only, and framework root resolution now handles packages that hide ./package.json through exports. Local validation: focused ManifestLoader vitest, broader egg-bundler vitest set, typecheck, lint, and git diff --check all passed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Ports a set of previously-reviewed bundler/core patches onto next, focusing on bundled-manifest loading/hardening, revised externalization behavior, and runtime hooks to consume bundled artifacts without disk manifest reads.
Changes:
- Add/extend
@eggjs/corebundled-manifest support viaManifestStore.fromBundle()and a global bundle-store hook used by bundler-generated entries. - Harden
@eggjs/egg-bundlermanifest loading: safer JSON/error handling, stronger path normalization across dependency layouts (incl. nested/hoisted/optional), and make manifest auto-generation opt-in by default. - Update bundler externals + entry generation behavior (bundle framework packages by default; externalize optional deps; single-mode emits worker entry only) and expand test coverage/fixtures.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/test/integration.test.ts | Updates integration expectations for externals (framework bundled by default). |
| tools/egg-bundler/test/fixtures/externals/basic-app/package.json | Expands fixture deps to cover new externals rules (optional deps, egg/helpers present). |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/esm-string-export/package.json | Adds fixture for string exports shape coverage. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/egg/package.json | Adds fixture package metadata for egg. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/@swc/helpers/package.json | Adds fixture package metadata for @swc/helpers. |
| tools/egg-bundler/test/ManifestLoader.test.ts | Adds comprehensive tests for manifest normalization, autogen defaults, exports entry resolution, and error surfacing. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Updates externals tests to match new rules (no ESM-only auto-externalization; optional deps externalized; framework deps bundled unless forced). |
| tools/egg-bundler/test/EntryGenerator.test.ts | Updates tests for worker-only entry generation in single mode. |
| tools/egg-bundler/src/scripts/generate-manifest.mjs | Allows options via argv or stdin; improves framework import for absolute paths; removes forced process.exit(0). |
| tools/egg-bundler/src/lib/ManifestLoader.ts | Switches autogen default to off, uses execaNode, adds normalization hardening, and wires ManifestStore.fromBundle(). |
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Revises externalization rules (optional deps, peer deps, native detection), removes ESM-only auto-externalization, adds caching. |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Removes agent entry generation; worker entry sets bundle store/module loader for runtime. |
| tools/egg-bundler/package.json | Refines exports to split types vs import; adds execa; updates devDeps. |
| packages/core/test/loader/manifest.test.ts | Adds coverage for bundle-store hook behavior and fromBundle() validation. |
| packages/core/src/loader/manifest.ts | Implements bundle-store hook keyed by baseDir; adds fromBundle() with validation. |
| packages/core/src/index.ts | Ensures global augmentation is included in type output. |
| packages/core/src/global.ts | Adds global typing for __EGG_BUNDLE_STORE__. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/egg-bundler/src/lib/ManifestLoader.ts`:
- Around line 455-463: In ManifestLoader.ts, update the package.json walk logic
around the candidate/access try-catch so the catch captures the thrown error
(e.g., catch (err)) and only suppresses it when err.code === 'ENOENT' (continue
walking to parent), otherwise rethrow the error; keep the existing parent ===
dir termination check and only fall back to throwing the original error for
non-ENOENT cases. This targets the candidate/path resolution block that
references candidate, dir, parent to avoid treating permission or other I/O
failures as "keep searching."
- Around line 79-81: Currently load() assigns this.#manifest immediately after
awaiting this.#normalize(data) before calling
ManifestStore.fromBundle(normalized, this.#baseDir); change the order so you
first call ManifestStore.fromBundle(normalized, this.#baseDir) and only assign
this.#manifest and this.#store after fromBundle resolves successfully; ensure
that if fromBundle rejects, neither this.#manifest nor this.#store are mutated
so subsequent calls to load() will retry and fail consistently.
- Around line 301-305: When pkg.exports is present, don't fall back to
pkg.module/pkg.main if `#resolveExportsEntry`(pkg.exports) returns undefined;
instead throw a clear error to mirror Node.js ERR_MODULE_NOT_EXPORTED behavior.
Update the logic around entryRel in ManifestLoader.ts so that: if pkg.exports
exists, call this.#resolveExportsEntry(pkg.exports) and if that returns
undefined raise an error (with context including package name/path) rather than
assigning entryRel = pkg.module ?? pkg.main; only perform the fallback to
pkg.module or pkg.main when pkg.exports is entirely absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad4447ed-1e18-4724-bceb-8de0ad7b6745
📒 Files selected for processing (2)
tools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/ManifestLoader.test.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Ports a set of review-fix patches for the bundler/manifest split work onto next, tightening bundled-manifest handling, updating externalization rules, and aligning the bundler’s emitted entries with single-mode behavior.
Changes:
- Add/extend bundled-manifest support in
@eggjs/core(ManifestStore.fromBundle, global bundle store hook, and related tests/typings). - Harden
@eggjs/egg-bundlermanifest loading/generation (normalization, subprocess invocation, error messages) and update externals resolution behavior (optional deps, framework bundling). - Emit only the worker entry by default (remove agent entry generation) and update tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ManifestLoader.ts | Async manifest IO, normalization hardening, subprocess generation via execaNode, and autoGenerate default change |
| tools/egg-bundler/src/scripts/generate-manifest.mjs | Accept options via argv or stdin; framework import specifier normalization |
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Update externalization rules; add caching and optionalDependencies handling |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Stop generating agent entry by default |
| tools/egg-bundler/package.json | Export map now declares types + import; add execa dependency |
| tools/egg-bundler/test/*.test.ts (+ fixtures) | Update expectations for externals, entries, and add ManifestLoader coverage |
| packages/core/src/loader/manifest.ts | Add bundle store hook keyed on baseDir; implement fromBundle validation |
| packages/core/src/global.ts | Add global typing for __EGG_BUNDLE_STORE__ |
| packages/core/test/loader/manifest.test.ts | Add coverage for bundle store hook + fromBundle() |
This was referenced May 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Source branch: origin/split/01-utils-bundle-module-loader
Base: origin/next @ 101ab97
Patch-equivalence checks showed several source commits had equivalent counterparts already on next, but review-fix deltas were still missing or had evolved under different SHAs. This PR ports the equivalent patch set onto latest next without merging the old split branch history.
Ported source commits / equivalent patches:
Already covered on next by equivalent/evolved commits and not re-applied directly:
Validation
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Tests