fix(utils): preserve bundled native import fallback#5924
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:
📝 WalkthroughWalkthroughAdds a lazy helper that builds an opaque native ChangesNative dynamic-import fallback
Sequence Diagram(s)sequenceDiagram
autonumber
participant ImportCaller as importModule()
participant BundleLoader as _bundleModuleLoader
participant NativeImport as getNativeDynamicImport()
participant Module as ESM module
ImportCaller->>BundleLoader: ask to load specifier
alt loader returns module
BundleLoader->>ImportCaller: module (short-circuit)
ImportCaller->>Module: return loaded module
else loader returns undefined
BundleLoader->>ImportCaller: undefined
ImportCaller->>NativeImport: call(nativeImport, fileUrl)
NativeImport->>Module: dynamic import(fileUrl)
Module->>ImportCaller: resolved module
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 #5924 +/- ##
==========================================
+ Coverage 85.04% 85.06% +0.01%
==========================================
Files 667 667
Lines 19128 19129 +1
Branches 3725 3725
==========================================
+ Hits 16268 16272 +4
+ Misses 2467 2465 -2
+ Partials 393 392 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
597e4d9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://403b48ae.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-06640e7b.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 introduces a mechanism to hide dynamic imports from bundlers like Turbopack by wrapping the import() call within a new Function constructor. This ensures that the fallback to native dynamic import remains opaque to static analysis and transformation tools when a bundle loader is active. The changes include the implementation of nativeDynamicImport in packages/utils/src/import.ts and the addition of integration tests and fixtures to verify the fallback behavior and the presence of the obfuscation in the source code. I have no feedback to provide.
Deploying egg-v3 with
|
| Latest commit: |
597e4d9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc491c95.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-06640e7b.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 the current code and only fix it if needed.
Inline comments:
In `@packages/utils/test/bundle-import.test.ts`:
- Around line 75-79: The regex in the test "hides dynamic import fallback from
bundled expression transforms" is line-ending sensitive; update the second
assert.match that currently uses a literal '\n' to use '\r?\n' so it matches
both LF and CRLF checkouts—locate the test block, the 'source' variable and the
assert.match that checks /if \(_bundleModuleLoader\) \{\n\s+obj = await
nativeDynamicImport\(fileUrl\);/ and replace the '\n' with '\r?\n' to make the
pattern platform-agnostic.
🪄 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: c8570142-b272-48d9-ad91-419c1712cfbe
📒 Files selected for processing (3)
packages/utils/src/import.tspackages/utils/test/bundle-import.test.tspackages/utils/test/fixtures/bundle-native-fallback/run.mjs
There was a problem hiding this comment.
Pull request overview
Fixes bundled-runtime behavior in @eggjs/utils’s importModule() so that when a bundle module loader is installed but returns undefined (miss), the code reliably falls back to Node’s native dynamic import rather than a bundler-rewritten import path.
Changes:
- Add an “opaque” native dynamic import helper (via
new Function) and use it for ESM fallback when a bundle loader is present. - Add a fixture + spawned Node test to verify bundle-loader miss falls through to native dynamic import.
- Add a source-shape guard test to prevent regressions from bundler expression rewrites.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/utils/src/import.ts | Uses an opaque native dynamic import fallback when a bundle loader is registered, preserving correct runtime behavior on bundle misses. |
| packages/utils/test/bundle-import.test.ts | Adds an integration-style spawn test for the fallback and a source-shape assertion guarding against bundler rewrites. |
| packages/utils/test/fixtures/bundle-native-fallback/run.mjs | New Node fixture that installs a miss-only bundle loader and exercises importModule() fallback behavior. |
killagu
force-pushed
the
agent/egg-dev/06640e7b
branch
2 times, most recently
from
b238c93 to
e522a08
Compare
killagu
force-pushed
the
agent/egg-dev/06640e7b
branch
from
e522a08 to
83c8d23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utils/src/import.ts`:
- Around line 473-478: Move the V8 coverage directive to the if-statement itself
so the branch miss is suppressed: replace the inline comment inside the
true-branch with a branch-targeted directive on the `if (_bundleModuleLoader)`
line (use `/* v8 ignore if */`), leaving the dynamic import logic (`obj = await
getNativeDynamicImport()(fileUrl);` and the `else` path `obj = await
import(fileUrl);`) unchanged; ensure the directive sits immediately after `if
(_bundleModuleLoader)` so the coverage tool recognizes and ignores that branch.
🪄 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: 6c02b2fd-60eb-44d6-90f9-b3db5dc46c6c
📒 Files selected for processing (3)
packages/utils/src/import.tspackages/utils/test/bundle-import.test.tspackages/utils/test/fixtures/bundle-native-fallback/run.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/utils/test/fixtures/bundle-native-fallback/run.mjs
killagu
force-pushed
the
agent/egg-dev/06640e7b
branch
from
83c8d23 to
de8f064
Compare
killagu
force-pushed
the
agent/egg-dev/06640e7b
branch
from
de8f064 to
597e4d9
Compare
Summary
Tests
Summary by CodeRabbit
New Features
Tests