fix(bundler): normalize tegg manifest paths#5932
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:
📝 WalkthroughWalkthroughThe PR implements and integrates a bundler-based module loader mechanism. It adds support for loading pre-bundled modules via ChangesBundler Module Loader Integration
Sequence DiagramsequenceDiagram
actor User as Application Code
participant Load as LoaderUtil.loadFile()
participant Global as globalThis
participant Bundle as __EGG_BUNDLE_MODULE_LOADER__
participant Dynamic as import()
User->>Load: loadFile(filePath)
Load->>Load: normalize path (\ to /)
Load->>Global: read __EGG_BUNDLE_MODULE_LOADER__
Global-->>Load: loader function
Load->>Bundle: call loader(normalizedPath)
alt Bundle Returns Module
Bundle-->>Load: module object
Load-->>User: parsed prototypes
else Bundle Returns null/undefined
Bundle-->>Load: null/undefined
Load->>Load: apply Windows file:// conversion
Load->>Dynamic: import(filePath)
Dynamic-->>Load: module exports
Load-->>User: parsed prototypes
else Bundle Throws Error
Bundle-->>Load: Error thrown
Load->>Load: createLoadError(filePath, error)
Load-->>User: wrapped Error with cause
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
Deploying egg with
|
| Latest commit: |
49408cc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3324b18c.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-85bc11ed.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5932 +/- ##
=======================================
Coverage 85.18% 85.19%
=======================================
Files 668 668
Lines 19284 19288 +4
Branches 3782 3784 +2
=======================================
+ Hits 16428 16432 +4
Misses 2464 2464
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 introduces a bundle module loader mechanism to tegg/core/loader and updates tools/egg-bundler to support path normalization for moduleReferences and moduleDescriptors. The changes include adding a global hook for module loading, updating the loadFile utility, and adding comprehensive tests for the new functionality. A review comment suggests improving error handling in the catch block to safely handle non-Error exceptions.
Deploying egg-v3 with
|
| Latest commit: |
49408cc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa037ac2.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-85bc11ed.egg-v3.pages.dev |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/core/loader/src/LoaderUtil.ts`:
- Around line 73-91: The bundle loader can return null to indicate "not bundled"
but the current fallback only checks exports === undefined causing a later
TypeError; update the check around the bundle result (the local variable exports
produced by (globalThis as BundleModuleGlobalThis).__EGG_BUNDLE_MODULE_LOADER__
invocation in LoaderUtil.ts) to treat both null and undefined as "not bundled"
(e.g., use exports == null or explicit exports === undefined || exports ===
null) so the dynamic import fallback (await import(filePath)) runs when the
bundle loader returns null.
🪄 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: d8b70aa8-a84f-446c-b4dc-5f3f7812fbb7
📒 Files selected for processing (5)
tegg/core/loader/src/LoaderUtil.tstegg/core/loader/test/Loader.test.tstools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/ManifestLoader.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/core/loader/src/LoaderUtil.ts`:
- Around line 73-75: The call to the global bundle loader in LoaderUtil.loadFile
is not wrapped in a try/catch so thrown values bypass the standard
"[tegg/loader] load ... failed" wrapper; update the code around the
__EGG_BUNDLE_MODULE_LOADER__ invocation in loadFile to catch any thrown value
(including non-Error), and rethrow a new Error that prefixes the original
message/stack with "[tegg/loader] load <originalFilePath> failed: " (or include
the original error message when no stack exists) so behavior matches the
import-failure handling currently used in the later catch block.
🪄 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: df572c9c-91e5-441a-bf15-9b21cb511433
📒 Files selected for processing (2)
tegg/core/loader/src/LoaderUtil.tstegg/core/loader/test/Loader.test.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the bundling pipeline and tegg loader integration so that tegg manifest paths and decorated files remain resolvable in a bundled runtime, and so tegg can load pre-bundled modules via the bundle module loader before falling back to dynamic import.
Changes:
- Normalize tegg
moduleReferences[].pathandmoduleDescriptors[].unitPathto consistent, app-relative (or moduleMap-normalized) paths during manifest normalization. - Expand
EntryGeneratorcoverage to include decorated files underapp/portfrom tegg manifest descriptors. - Enhance
LoaderUtil.loadFile()to consult a registeredglobalThis.__EGG_BUNDLE_MODULE_LOADER__first, with tests covering the fast-path and fallback behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ManifestLoader.ts | Normalize tegg module reference/descriptor paths via a shared helper to ensure consistent keys at runtime. |
| tools/egg-bundler/test/ManifestLoader.test.ts | Add regression test asserting tegg path normalization and decorated-file resolution. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Add test ensuring app/port tegg-decorated files are included in generated worker imports. |
| tegg/core/loader/src/LoaderUtil.ts | Add bundle-module-loader fast path before dynamic import for pre-bundled file loading. |
| tegg/core/loader/test/Loader.test.ts | Add tests for bundle-module-loader loading and fallback, plus global cleanup between tests. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/core/loader/src/LoaderUtil.ts`:
- Around line 88-90: The import failure wrapper is using filePath (which can be
a file:// URL on Windows) when calling createLoadError; change the catch to call
createLoadError with originalFilePath instead so the error uses the
caller-facing path. Locate the catch block around the dynamic import (the line
with "exports = await import(filePath);" and the subsequent throw
createLoadError(...)) and replace the second argument from filePath to
originalFilePath; keep the original caught error (e) passed through unchanged.
🪄 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: e465c7db-34d3-4a51-9328-1ccd149139dd
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/utils/src/import.tstegg/core/loader/src/LoaderUtil.tstegg/core/loader/test/Loader.test.tstools/egg-bundler/src/lib/EntryGenerator.ts
| exports = await import(filePath); | ||
| } catch (e: unknown) { | ||
| throw createLoadError(filePath, e); |
There was a problem hiding this comment.
Preserve original path in import-failure errors.
At Line 90, wrapping with filePath can emit a file://... URL on Windows, which makes troubleshooting less clear than the caller-facing path. Use originalFilePath in the error wrapper.
Suggested patch
try {
exports = await import(filePath);
} catch (e: unknown) {
- throw createLoadError(filePath, e);
+ throw createLoadError(originalFilePath, e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exports = await import(filePath); | |
| } catch (e: unknown) { | |
| throw createLoadError(filePath, e); | |
| exports = await import(filePath); | |
| } catch (e: unknown) { | |
| throw createLoadError(originalFilePath, e); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tegg/core/loader/src/LoaderUtil.ts` around lines 88 - 90, The import failure
wrapper is using filePath (which can be a file:// URL on Windows) when calling
createLoadError; change the catch to call createLoadError with originalFilePath
instead so the error uses the caller-facing path. Locate the catch block around
the dynamic import (the line with "exports = await import(filePath);" and the
subsequent throw createLoadError(...)) and replace the second argument from
filePath to originalFilePath; keep the original caught error (e) passed through
unchanged.
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores