fix(utils): resolve bundle-inlined modules in importResolve#5971
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBundle-mode interception in importResolve
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. Comment |
Deploying egg with
|
| Latest commit: |
b6724b3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ac704799.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-utils-import-resolve-bun.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 importResolve in packages/utils/src/import.ts to support bundle-only modules by checking a registered bundle module loader when on-disk resolution fails, returning the path as the canonical key. It also adds corresponding unit tests. The review feedback highlights a key limitation: the current implementation does not correctly resolve relative paths for bundle-only modules because it only checks the unresolved path against the loader. The reviewer suggests resolving relative paths against the provided paths option, including checking common file extensions, and provides code suggestions and test cases to address this issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const bundleModuleLoader = globalThis.__EGG_BUNDLE_MODULE_LOADER__; | ||
| if (bundleModuleLoader && bundleModuleLoader(normalizeBundleModulePath(filepath)) !== undefined) { | ||
| debug('[importResolve:bundle] %o => %o', filepath, filepath); | ||
| return filepath; | ||
| } |
There was a problem hiding this comment.
When resolving a relative path (e.g., ./foo) to a bundle-only module, the current implementation only checks the unresolved filepath against the bundle loader. Since the bundle loader typically indexes modules by their absolute canonical paths, this check will fail. We should resolve the relative path against the provided paths option and check those resolved absolute paths (along with common extensions) against the bundle loader to ensure correct resolution.
const bundleModuleLoader = globalThis.__EGG_BUNDLE_MODULE_LOADER__;
if (bundleModuleLoader) {
const normalizedFilepath = normalizeBundleModulePath(filepath);
if (bundleModuleLoader(normalizedFilepath) !== undefined) {
debug('[importResolve:bundle] %o => %o', filepath, filepath);
return filepath;
}
if (isRelativePath(filepath)) {
for (const p of paths) {
const resolvedPath = path.resolve(p, filepath);
const normalizedResolved = normalizeBundleModulePath(resolvedPath);
if (bundleModuleLoader(normalizedResolved) !== undefined) {
debug('[importResolve:bundle:relative] %o => %o', filepath, resolvedPath);
return resolvedPath;
}
for (const ext of [".js", ".ts", ".mjs", ".cjs", "/index.js", "/index.ts"]) {
if (bundleModuleLoader(normalizedResolved + ext) !== undefined) {
debug('[importResolve:bundle:relative] %o => %o', filepath, resolvedPath + ext);
return resolvedPath + ext;
}
}
}
}
}There was a problem hiding this comment.
Thanks, but I am intentionally not resolving relative paths here. This fallback is deliberately symmetric with importModule, which keys the loader on normalizeBundleModulePath(filepath) using the raw (unresolved) filepath. importResolve returns that same filepath as the canonical key precisely so a subsequent importModule(filepath) looks the loader up with an identical key.
Resolving ./foo to an absolute path before the lookup would make importResolve return a different key than the one importModule keys on, breaking that contract. The bundle loader itself owns the path namespace and decides what it recognizes; both functions hand it the same path-as-passed. So this is by design rather than an oversight, and I am leaving the relative-path test out for the same reason.
| it('importResolve returns the path as canonical key for bundle-only modules', () => { | ||
| const seen: string[] = []; | ||
| setBundleModuleLoader((p) => { | ||
| seen.push(p); | ||
| return p === 'virtual/not-on-disk' ? { virtual: true } : undefined; | ||
| }); | ||
|
|
||
| // The module is inlined into the bundle and has no source on disk, but the | ||
| // loader recognizes it, so importResolve hands it back unchanged. | ||
| assert.equal(importResolve('virtual/not-on-disk'), 'virtual/not-on-disk'); | ||
| assert.ok(seen.includes('virtual/not-on-disk')); | ||
| }); | ||
|
|
||
| it('importResolve normalizes Windows-style paths before the bundle lookup', () => { | ||
| setBundleModuleLoader((p) => (p === 'virtual/win/mod' ? { windows: true } : undefined)); | ||
|
|
||
| const filepath = 'virtual/win/mod'.split(path.posix.sep).join(path.win32.sep); | ||
| assert.equal(importResolve(filepath), filepath); | ||
| }); | ||
|
|
||
| it('importResolve does not consult the loader once on-disk resolution succeeds', () => { | ||
| let called = false; | ||
| setBundleModuleLoader(() => { | ||
| called = true; | ||
| return { hit: true }; | ||
| }); | ||
|
|
||
| const resolved = importResolve(getFilepath('esm')); | ||
| assert.ok(resolved.endsWith('/fixtures/esm/index.js')); | ||
| assert.equal(called, false); | ||
| }); | ||
|
|
||
| it('importResolve still throws for missing modules when no loader is registered', () => { | ||
| assert.throws(() => importResolve('virtual/not-on-disk')); | ||
| }); |
There was a problem hiding this comment.
Add a test case to verify that importResolve can correctly resolve relative paths for bundle-only modules against the paths option.
it('importResolve returns the path as canonical key for bundle-only modules', () => {
const seen: string[] = [];
setBundleModuleLoader((p) => {
seen.push(p);
return p === 'virtual/not-on-disk' ? { virtual: true } : undefined;
});
// The module is inlined into the bundle and has no source on disk, but the
// loader recognizes it, so importResolve hands it back unchanged.
assert.equal(importResolve('virtual/not-on-disk'), 'virtual/not-on-disk');
assert.ok(seen.includes('virtual/not-on-disk'));
});
it('importResolve resolves relative paths for bundle-only modules against paths option', () => {
const absolutePath = path.resolve(process.cwd(), 'virtual/relative/mod.js');
const normalizedAbsolute = absolutePath.split(path.win32.sep).join(path.posix.sep);
setBundleModuleLoader((p) => (p === normalizedAbsolute ? { relative: true } : undefined));
assert.equal(importResolve('./relative/mod', { paths: [path.resolve(process.cwd(), 'virtual')] }), absolutePath);
});
it('importResolve normalizes Windows-style paths before the bundle lookup', () => {
setBundleModuleLoader((p) => (p === 'virtual/win/mod' ? { windows: true } : undefined));
const filepath = 'virtual/win/mod'.split(path.posix.sep).join(path.win32.sep);
assert.equal(importResolve(filepath), filepath);
});
it('importResolve does not consult the loader once on-disk resolution succeeds', () => {
let called = false;
setBundleModuleLoader(() => {
called = true;
return { hit: true };
});
const resolved = importResolve(getFilepath('esm'));
assert.ok(resolved.endsWith('/fixtures/esm/index.js'));
assert.equal(called, false);
});
it('importResolve still throws for missing modules when no loader is registered', () => {
assert.throws(() => importResolve('virtual/not-on-disk'));
});There was a problem hiding this comment.
Skipping the relative-path test for the reason explained on the import.ts thread: the fallback is intentionally symmetric with importModule and keys the loader on the raw path-as-passed, so resolving relatives here would break the canonical-key contract. The other suggested cases are already covered by the existing tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5971 +/- ##
=======================================
Coverage 85.57% 85.57%
=======================================
Files 669 669
Lines 19849 19853 +4
Branches 3923 3924 +1
=======================================
+ Hits 16985 16990 +5
+ Misses 2475 2474 -1
Partials 389 389 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 `@packages/utils/test/bundle-import.test.ts`:
- Around line 166-168: The assertion on the resolved path ending with
'/fixtures/esm/index.js' uses hardcoded POSIX separators which causes test
failures on Windows. Fix this by building the expected path string dynamically
using path utilities (such as path.sep or path.join) to ensure the correct path
separator is used for the current platform, or by normalizing both the resolved
and expected paths before comparison.
🪄 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: 387c3420-32e3-4738-ab79-161b5a055568
📒 Files selected for processing (2)
packages/utils/src/import.tspackages/utils/test/bundle-import.test.ts
Deploying egg-v3 with
|
| Latest commit: |
b6724b3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7475084e.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-utils-import-resolve-bun.egg-v3.pages.dev |
In bundle mode a module's source may not exist on disk, so on-disk resolution fails and importResolve falls into import.meta.resolve, which is unavailable in the bundled runtime. Add a fallback that runs after disk resolution fails and before import.meta.resolve: when a bundle module loader is registered and recognizes the normalized path, return the path unchanged as the canonical key, mirroring importModule. The fallback only takes effect when __EGG_BUNDLE_MODULE_LOADER__ is registered; behavior is unchanged otherwise. Adds importResolve coverage for the registered, Windows-path, disk-hit, and unregistered cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
killagu
force-pushed
the
fix-utils-import-resolve-bundle
branch
from
30f2d5e to
ffc6286
Compare
Use a separator-agnostic regex instead of a hardcoded POSIX-separator endsWith() so the test holds on Windows too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Motivation
In bundle mode a module's source may not exist on disk — it is inlined into the bundle.
importResolveruns through its on-disk resolution paths (absolute / relative /node_modules), and on failure falls intoimport.meta.resolve, which is unavailable in the bundled runtime. As a result, resolving a bundle-only module throws even thoughimportModuleitself already knows how to serve it via the registered bundle module loader.Scope
Add a bundle fallback to
importResolve, symmetric with the existing one inimportModule:import.meta.resolve.globalThis.__EGG_BUNDLE_MODULE_LOADER__is registered and recognizesnormalizeBundleModulePath(filepath), the path is treated as already resolved and returned unchanged as the canonical key.importModule.The fallback only takes effect when a loader is registered. When no loader is registered, behavior is unchanged (the unregistered case still throws / falls back exactly as before) — covered by an explicit assertion.
Test evidence
Added
importResolvecoverage topackages/utils/test/bundle-import.test.ts:This PR has no dependency on the other bundle-startup PRs and can land first.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests