fix(bundler): start manifest app in metadataOnly mode#5910
Conversation
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe manifest generator starts the framework with ChangesMetadata-Only Manifest Generation
Sequence DiagramsequenceDiagram
participant Script as GenerateManifest.mjs
participant Framework as framework.mjs
participant Loader as loader.generateManifest
participant FS as Filesystem
participant Proc as Process
Script->>Framework: start({ ..., metadataOnly: true })
Framework->>Loader: generateManifest()
Loader-->>Framework: manifest JSON
Framework-->>Script: manifest result
Script->>FS: write .egg/manifest.json
Script->>Proc: flush stdout/stderr
Script->>Proc: process.exit(0)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Deploying egg with
|
| Latest commit: |
65e7513
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc044c91.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-84a193fb.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5910 +/- ##
==========================================
- Coverage 85.03% 85.03% -0.01%
==========================================
Files 665 665
Lines 19108 19108
Branches 3719 3719
==========================================
- Hits 16249 16248 -1
- Misses 2466 2467 +1
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request modifies the generate-manifest script to enable metadataOnly mode when starting the application. This change ensures that lifecycle hooks, such as beforeClose, are skipped during the manifest generation process. Additionally, a new test file generate-manifest.test.ts has been introduced to validate that the metadataOnly flag is correctly applied and that the expected files are generated or skipped. I have no feedback to provide.
Deploying egg-v3 with
|
| Latest commit: |
65e7513
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99b4d7fd.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-84a193fb.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR ensures the bundler’s manifest-generation subprocess starts the Egg app in metadataOnly mode to avoid triggering normal shutdown/beforeClose logic during manifest generation, and adds a regression test that reproduces the failure mode.
Changes:
- Start the manifest app with
metadataOnly: trueingenerate-manifest.mjs. - Add a minimal, synthetic framework app repro to validate
metadataOnlyis passed and that shutdown hooks are skipped. - Assert that
.egg/manifest.jsonis written and the “beforeClose” failure path is not hit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/egg-bundler/src/scripts/generate-manifest.mjs | Passes metadataOnly: true when starting the framework app for manifest generation. |
| tools/egg-bundler/test/generate-manifest.test.ts | Adds a subprocess-based regression test for the metadataOnly startup behavior. |
41b984c to
ba3dd3b
Compare
ba3dd3b to
43f924d
Compare
43f924d to
65e7513
Compare
## Summary Fixes the current cnpmcore production bundle blockers seen on next after #5910: - generate worker entries with a portable framework import instead of an absolute node_modules/egg path - inline the bundle module loader registration so the generated worker does not depend on @eggjs/utils - provide runtime exports for EggAppConfig and Logger so non-import type app code survives static export validation - externalize packages that have missing optional peer dependencies, and add the missing optional peers as externals, covering leoric optional sql.js / mysql / sqlite3 paths ## Validation - pnpm vitest run test/EntryGenerator.test.ts test/ExternalsResolver.test.ts in tools/egg-bundler - pnpm vitest run packages/egg/test/index.test.ts - pnpm --filter egg run typecheck - pnpm --filter @eggjs/egg-bundler run typecheck - full pnpm run build was run during verification after the implementation changes - cnpmcore validation with local egg CLI/dist shim: node ../egg/tools/egg-bin/bin/run.js bundle --mode production --output dist-bundle generated dist-bundle successfully <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Bundler now externalizes missing optional peer dependencies for more correct builds. * **Bug Fixes** * Configuration and logger exports are exposed at runtime so apps start reliably. * **Chores** * Removed an unused dependency. * Standardized the bundler’s runtime module-loader approach. * **Documentation** * Updated bundling docs to reflect new module-loader and startup behavior. * **Tests** * Added/updated tests covering externals resolution and generated runtime hooks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Tests
Refs: EGG-18
Summary by CodeRabbit