docs(bundler): clarify metadata manifest and externals#5915
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocuments egg-bundler behavior: when Changesegg-bundler documentation updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 5/8 reviews remaining, refill in 20 minutes and 25 seconds.Comment |
Deploying egg with
|
| Latest commit: |
1e82a25
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d82dfb5a.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-doc-70eb664b.egg-cci.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for the egg-bundler tool, specifically detailing the manifest auto-generation process in metadataOnly mode and refining the list of externalized dependencies in the output structure and wiki. Feedback was provided to clarify that the automatic detection of native addons and missing optional peers by the ExternalsResolver applies specifically to root-level dependencies to prevent potential misconceptions.
| `optionalDependencies`, packages with native addons/native binaries, packages | ||
| whose optional peer dependencies are missing, and those missing optional peer | ||
| package names. They must be installed alongside the bundle — typically by |
There was a problem hiding this comment.
The ExternalsResolver implementation only iterates over the application's root dependencies to auto-detect native modules and missing optional peers. It does not recursively scan the entire dependency tree. Clarifying that this auto-detection applies specifically to root-level packages prevents the misconception that all native modules in the tree are automatically handled.
| `optionalDependencies`, packages with native addons/native binaries, packages | |
| whose optional peer dependencies are missing, and those missing optional peer | |
| package names. They must be installed alongside the bundle — typically by | |
| `optionalDependencies`, root packages with native addons/native binaries, root | |
| packages whose optional peer dependencies are missing, and those missing optional peer | |
| package names. They must be installed alongside the bundle — typically by |
| `optionalDependencies`, native addons, packages with missing optional peers, | ||
| and the missing optional peer package names are external. |
There was a problem hiding this comment.
Consistent with the changes in output-structure.md, it's important to specify that the automatic externalization of native addons and packages with missing optional peers is limited to root dependencies. Deep dependencies with these characteristics are not automatically detected by the current ExternalsResolver logic.
| `optionalDependencies`, native addons, packages with missing optional peers, | |
| and the missing optional peer package names are external. | |
| `optionalDependencies`, root packages with native addons, root packages with | |
| missing optional peers, and the missing optional peer package names are external. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wiki/packages/egg-bundler.md (1)
46-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame clarity issue: missing optional peer dependencies.
This section contains the same ambiguous phrasing as in
tools/egg-bundler/docs/output-structure.md(lines 64-66): "packages with missing optional peers, and the missing optional peer package names". Please apply the same clarification to maintain consistency across documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/packages/egg-bundler.md` around lines 46 - 48, Rephrase the ambiguous clause in the sentence that lists externals so it clearly distinguishes the package entries that have unresolved optional peer deps from the actual package names of those missing peers; replace "packages with missing optional peers, and the missing optional peer package names are external" with wording like "packages that declare optional peerDependencies which could not be resolved (these packages are treated as external), and the names of the missing optional peer packages themselves are also treated as external" so it matches the clarification applied in tools/egg-bundler/docs/output-structure.md; update the single sentence containing Explicit `externals.force` entries, root `peerDependencies`, root `optionalDependencies`, native addons, ... to use this clearer phrasing.
🤖 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/docs/output-structure.md`:
- Around line 64-72: The sentence about "packages whose optional peer
dependencies are missing, and those missing optional peer package names" is
ambiguous; update the text in output-structure.md to either merge into a single
clear phrase ("packages whose optional peer dependencies are missing") if they
mean the same thing, or explicitly describe the two distinct cases (1) packages
that list optional peer dependencies but those peer packages are not installed,
and (2) packages that only reference optional peer package names without
providing the actual packages — and clarify how each case is treated with
respect to bundling/externalization and ExternalsResolver/externals.force
behavior; reference the terms optionalDependencies, optional peer dependencies,
ExternalsResolver, and externals.force when clarifying.
---
Duplicate comments:
In `@wiki/packages/egg-bundler.md`:
- Around line 46-48: Rephrase the ambiguous clause in the sentence that lists
externals so it clearly distinguishes the package entries that have unresolved
optional peer deps from the actual package names of those missing peers; replace
"packages with missing optional peers, and the missing optional peer package
names are external" with wording like "packages that declare optional
peerDependencies which could not be resolved (these packages are treated as
external), and the names of the missing optional peer packages themselves are
also treated as external" so it matches the clarification applied in
tools/egg-bundler/docs/output-structure.md; update the single sentence
containing Explicit `externals.force` entries, root `peerDependencies`, root
`optionalDependencies`, native addons, ... to use this clearer phrasing.
🪄 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: 26406707-3173-4596-908d-8eab94dc58db
📒 Files selected for processing (4)
tools/egg-bundler/README.mdtools/egg-bundler/docs/output-structure.mdwiki/log.mdwiki/packages/egg-bundler.md
Deploying egg-v3 with
|
| Latest commit: |
1e82a25
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5e4e748b.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-doc-70eb664b.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
Updates Egg bundler documentation to better reflect current manifest auto-generation behavior and how ExternalsResolver decides what stays external versus bundled.
Changes:
- Documented startup manifest auto-generation when
<baseDir>/.egg/manifest.jsonis missing (viametadataOnly: truerun). - Clarified which dependency categories are externalized, including handling for missing optional peer dependencies.
- Updated the wiki package page metadata and added a wiki log entry for the documentation refinement.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wiki/packages/egg-bundler.md | Updates wiki page metadata date and expands notes on manifest generation + externals rules. |
| wiki/log.md | Adds a new log entry describing the bundler docs refinement. |
| tools/egg-bundler/docs/output-structure.md | Refines externals explanation to include missing optional peer dependency behavior. |
| tools/egg-bundler/README.md | Adds a short explanation of manifest auto-generation using metadataOnly mode. |
| package names. They must be installed alongside the bundle — typically by | ||
| copying the app's `package.json` next to `worker.js` and running | ||
| `npm ci --omit=dev`, or by deploying into an environment where these | ||
| dependencies are already installed. ESM-only packages, `egg`, `@swc/helpers`, | ||
| and `@eggjs/*` packages are bundled by default unless `ExternalsResolver` | ||
| externalizes them through `externals.force`, dependency metadata, or native | ||
| addon detection. |
| - Explicit `externals.force` entries, root `peerDependencies`, root | ||
| `optionalDependencies`, native addons, packages with missing optional peers, | ||
| and the missing optional peer package names are external. | ||
| - ESM-only packages, `egg`, `@swc/helpers`, and `@eggjs/*` packages are bundled | ||
| by default unless force-external or dependency/native-addon rules apply. |
killagu
force-pushed
the
agent/egg-doc/70eb664b
branch
from
1afdfa9 to
2149996
Compare
killagu
force-pushed
the
agent/egg-doc/70eb664b
branch
from
2149996 to
04a2ac0
Compare
| with `metadataOnly: true` to generate it. This skips the agent and normal boot | ||
| lifecycle, runs `loadMetadata()` hooks, and skips registered `beforeClose` | ||
| hooks when the temporary app closes. |
| lifecycle, runs `loadMetadata()` hooks, and does not run registered | ||
| `beforeClose` hooks while closing the temporary app. |
Summary: document metadataOnly manifest generation, clarify externals behavior for missing optional peer dependencies, and update the Egg Bundler wiki/log. Validation: git diff --check passed. pnpm exec oxfmt --check could not run because oxfmt is not installed in this checkout (node_modules is missing).
Summary by CodeRabbit