fix(bundler): restore tegg manifest runtime paths#5937
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR modifies ChangesRuntime Manifest Path Restoration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5937 +/- ##
==========================================
- Coverage 85.19% 85.18% -0.01%
==========================================
Files 668 668
Lines 19288 19288
Branches 3784 3784
==========================================
- Hits 16432 16431 -1
- Misses 2464 2465 +1
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 implements runtime restoration of relative paths in the tegg manifest to absolute paths, enabling correct loading of bundled modules. It includes a new test case for verification. Feedback recommends optimizing the manifest cloning process in __restoreBundleManifest to avoid performance overhead from JSON serialization during application startup.
| const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => { | ||
| const restored = JSON.parse(JSON.stringify(manifest)); | ||
| const tegg = restored.extensions?.tegg; | ||
| if (tegg?.moduleReferences) { | ||
| tegg.moduleReferences = tegg.moduleReferences.map((ref) => ({ | ||
| ...ref, | ||
| path: __restoreBundleRuntimePath(ref.path), | ||
| })); | ||
| } | ||
| if (tegg?.moduleDescriptors) { | ||
| tegg.moduleDescriptors = tegg.moduleDescriptors.map((desc) => ({ | ||
| ...desc, | ||
| unitPath: __restoreBundleRuntimePath(desc.unitPath), | ||
| })); | ||
| } | ||
| return restored; | ||
| }; |
There was a problem hiding this comment.
The __restoreBundleManifest function is executed at application startup. Using JSON.parse(JSON.stringify(manifest)) to deep clone the entire manifest can be quite inefficient, especially for large applications where the manifest might be several megabytes.
Since we only need to modify specific paths within the tegg extension, we can optimize this by using an early return if the extension is missing and performing a shallow clone of the top-level structure and the tegg extension specifically. This avoids the overhead of stringifying and re-parsing the whole object.
const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => {
const tegg = manifest.extensions?.tegg;
if (!tegg) return manifest;
const restoredTegg = { ...tegg };
if (restoredTegg.moduleReferences) {
restoredTegg.moduleReferences = restoredTegg.moduleReferences.map((ref: any) => ({
...ref,
path: __restoreBundleRuntimePath(ref.path),
}));
}
if (restoredTegg.moduleDescriptors) {
restoredTegg.moduleDescriptors = restoredTegg.moduleDescriptors.map((desc: any) => ({
...desc,
unitPath: __restoreBundleRuntimePath(desc.unitPath),
}));
}
return {
...manifest,
extensions: {
...manifest.extensions,
tegg: restoredTegg,
},
};
};
Deploying egg with
|
| Latest commit: |
43440e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1afb3faf.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-c4dbef28.egg-cci.pages.dev |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to restore bundle runtime paths within the Tegg manifest, ensuring that controller, service, and repository modules are correctly loaded using runtime paths. The changes include a new helper function __restoreBundleManifest and corresponding updates to the manifest loading logic in EntryGenerator.ts, along with a new test case to verify the path restoration. The review comment correctly identifies a performance concern regarding the use of JSON.parse(JSON.stringify()) for deep cloning the manifest and provides an optimized, more performant alternative for cloning only the necessary tegg extension data.
| const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => { | ||
| const restored = JSON.parse(JSON.stringify(manifest)); | ||
| const tegg = restored.extensions?.tegg; | ||
| if (tegg?.moduleReferences) { | ||
| tegg.moduleReferences = tegg.moduleReferences.map((ref) => ({ | ||
| ...ref, | ||
| path: __restoreBundleRuntimePath(ref.path), | ||
| })); | ||
| } | ||
| if (tegg?.moduleDescriptors) { | ||
| tegg.moduleDescriptors = tegg.moduleDescriptors.map((desc) => ({ | ||
| ...desc, | ||
| unitPath: __restoreBundleRuntimePath(desc.unitPath), | ||
| })); | ||
| } | ||
| return restored; | ||
| }; |
There was a problem hiding this comment.
Using JSON.parse(JSON.stringify(manifest)) to deep clone the entire manifest at runtime is inefficient, especially as the manifest (particularly fileDiscovery and resolveCache) can grow quite large in complex applications. Since only the tegg extension data requires path restoration, it is more performant to perform a shallow clone of the manifest and only update the specific tegg properties. This avoids the overhead of serializing and re-parsing the bulk of the manifest data during application startup.
const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => {
const tegg = manifest.extensions?.tegg as any;
if (!tegg) return manifest;
const restoredTegg = { ...tegg };
if (tegg.moduleReferences) {
restoredTegg.moduleReferences = tegg.moduleReferences.map((ref: any) => ({
...ref,
path: __restoreBundleRuntimePath(ref.path),
}));
}
if (tegg.moduleDescriptors) {
restoredTegg.moduleDescriptors = tegg.moduleDescriptors.map((desc: any) => ({
...desc,
unitPath: __restoreBundleRuntimePath(desc.unitPath),
}));
}
return {
...manifest,
extensions: {
...manifest.extensions,
tegg: restoredTegg,
},
} as any;
};
Deploying egg-v3 with
|
| Latest commit: |
43440e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e326a6f9.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-c4dbef28.egg-v3.pages.dev |
There was a problem hiding this comment.
Code Review
This pull request introduces logic to restore runtime paths for the tegg extension in the bundle manifest, ensuring that module references and descriptors are correctly resolved relative to the output directory. It includes new helper functions for manifest restoration and a comprehensive test case to validate the loading of bundled modules. A review comment suggests optimizing the manifest cloning process by using object spreading instead of JSON.parse(JSON.stringify()) to improve performance during application startup.
| const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => { | ||
| const restored = JSON.parse(JSON.stringify(manifest)); | ||
| const tegg = restored.extensions?.tegg; | ||
| if (tegg?.moduleReferences) { | ||
| tegg.moduleReferences = tegg.moduleReferences.map((ref) => ({ | ||
| ...ref, | ||
| path: __restoreBundleRuntimePath(ref.path), | ||
| })); | ||
| } | ||
| if (tegg?.moduleDescriptors) { | ||
| tegg.moduleDescriptors = tegg.moduleDescriptors.map((desc) => ({ | ||
| ...desc, | ||
| unitPath: __restoreBundleRuntimePath(desc.unitPath), | ||
| })); | ||
| } | ||
| return restored; | ||
| }; |
There was a problem hiding this comment.
Using JSON.parse(JSON.stringify(manifest)) for deep cloning is inefficient, especially for large manifests which can be several megabytes in size. This adds unnecessary CPU and memory overhead during application startup. Since only the tegg extension paths need to be restored, a more efficient approach is to use object spreading to create a shallow copy of the manifest and only deep-clone the tegg extension object.
const __restoreBundleManifest = (manifest: typeof MANIFEST_DATA) => {
const tegg = (manifest.extensions as any)?.tegg;
if (!tegg) return manifest;
return {
...manifest,
extensions: {
...manifest.extensions,
tegg: {
...tegg,
moduleReferences: tegg.moduleReferences?.map((ref: any) => ({
...ref,
path: __restoreBundleRuntimePath(ref.path),
})),
moduleDescriptors: tegg.moduleDescriptors?.map((desc: any) => ({
...desc,
unitPath: __restoreBundleRuntimePath(desc.unitPath),
})),
},
},
};
};There was a problem hiding this comment.
Pull request overview
This PR fixes bundled-worker runtime behavior for Tegg by restoring moduleReferences/moduleDescriptors paths in the inlined manifest to runtime output paths, ensuring decorated modules are loaded via the bundle module loader at runtime.
Changes:
- Restore Tegg
moduleReferences[].pathandmoduleDescriptors[].unitPathto runtime (__outputDir-based) absolute paths inside the generated worker entry. - Keep existing absolute-alias behavior so original (bundle-time) absolute paths can still be resolved through the bundle loader.
- Add a regression test and update the canonical worker snapshot to cover bundled controller/service/repository decorated files and public metadata consumption.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/EntryGenerator.ts | Generates a runtime-restored manifest for Tegg paths before registering the bundle store. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Adds a runtime regression ensuring decorated files load via runtime paths and original absolute aliases still work. |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Updates snapshot to reflect the new runtime manifest restoration logic in the generated worker. |
| const __APP_RESOLVE_CACHE_ALIASES: Array<[string, string]> = ${appResolveCacheAliases}; | ||
| const __restoreBundleRuntimePath = (filepath: string) => { | ||
| if (!filepath || path.isAbsolute(filepath)) return filepath; | ||
| return path.resolve(__outputDir, filepath); |
| const restored = JSON.parse(JSON.stringify(manifest)); | ||
| const tegg = restored.extensions?.tegg; | ||
| if (tegg?.moduleReferences) { | ||
| tegg.moduleReferences = tegg.moduleReferences.map((ref) => ({ | ||
| ...ref, | ||
| path: __restoreBundleRuntimePath(ref.path), | ||
| })); | ||
| } | ||
| if (tegg?.moduleDescriptors) { | ||
| tegg.moduleDescriptors = tegg.moduleDescriptors.map((desc) => ({ | ||
| ...desc, | ||
| unitPath: __restoreBundleRuntimePath(desc.unitPath), | ||
| })); | ||
| } | ||
| return restored; |
Summary
Tests
Summary by CodeRabbit
Bug Fixes
Tests