fix(bundler): externalize native optional wrappers#5919
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 ignored due to path filters (5)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds detection of native binaries inside a package's optionalDependencies: ExternalsResolver now resolves each optional dependency, reads its package.json, and externalizes the parent package if any optional dependency contains native indicators. ChangesNative optional-dependency native detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5919 +/- ##
=======================================
Coverage 85.04% 85.04%
=======================================
Files 667 667
Lines 19123 19123
Branches 3723 3723
=======================================
Hits 16263 16263
Misses 2467 2467
Partials 393 393 ☔ 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 logic to detect native binaries within optional dependencies, ensuring that wrapper packages are correctly externalized if any of their optional dependencies are native. This is implemented via a new #hasNativeOptionalDependency method in ExternalsResolver, accompanied by relevant test cases and fixtures. A review comment suggests reordering the checks in the resolution logic to prioritize the faster #hasNativeBinary check over the more expensive #hasNativeOptionalDependency check to improve performance through earlier short-circuiting.
Deploying egg with
|
| Latest commit: |
3daeece
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://85ab7720.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-70acad08.egg-cci.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/egg-bundler/src/lib/ExternalsResolver.ts (1)
112-120: ⚡ Quick winConfirm whether one hop of optional-dep scanning is enough.
This only checks each immediate optional dependency with
#hasNativeBinary(). A chain likewrapper-a -> optional wrapper-b -> optional native-cstill returnsfalseforwrapper-a, so another native optional wrapper would be inlined. If wrapper-of-wrapper packages are in scope here, recurse into#hasNativeOptionalDependency(depDir, depPkg)with a visited set and add a fixture for that shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ExternalsResolver.ts` around lines 112 - 120, The current `#hasNativeOptionalDependency` only checks immediate optionalDependencies and misses chains (e.g., wrapper-a -> optional wrapper-b -> optional native-c); update `#hasNativeOptionalDependency` to recursively scan optional dependencies by calling itself for each optional dep (after resolving with `#findPackageDir` and reading with `#readPackageJson`), track visited package dirs/names to avoid cycles, and return true if either `#hasNativeBinary`(...) or the recursive `#hasNativeOptionalDependency`(...) on a dependency returns true; also add a unit fixture that models a wrapper-of-wrapper scenario to verify the recursion and visited-set handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/egg-bundler/src/lib/ExternalsResolver.ts`:
- Around line 112-120: The current `#hasNativeOptionalDependency` only checks
immediate optionalDependencies and misses chains (e.g., wrapper-a -> optional
wrapper-b -> optional native-c); update `#hasNativeOptionalDependency` to
recursively scan optional dependencies by calling itself for each optional dep
(after resolving with `#findPackageDir` and reading with `#readPackageJson`), track
visited package dirs/names to avoid cycles, and return true if either
`#hasNativeBinary`(...) or the recursive `#hasNativeOptionalDependency`(...) on a
dependency returns true; also add a unit fixture that models a
wrapper-of-wrapper scenario to verify the recursion and visited-set handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 043ba6a3-1fdc-4964-b5fb-ffb7d33f2416
⛔ Files ignored due to path filters (5)
tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-platform/addon.nodeis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-platform/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-wrapper/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-js-dep/package.jsonis excluded by!**/node_modules/**tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-js-wrapper/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (3)
tools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/fixtures/externals/basic-app/package.json
Deploying egg-v3 with
|
| Latest commit: |
3daeece
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7a2d85f4.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-70acad08.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
Updates ExternalsResolver in tools/egg-bundler to classify “wrapper” packages as externals when their installed optionalDependencies exhibit native-addon signals (e.g., *.node, binding.gyp, prebuilds/, native install scripts). This tightens bundling correctness for cases like @cnpmjs/packument where the wrapper is JS but optionally installs a native platform package.
Changes:
- Add optional-dependency scanning (
#hasNativeOptionalDependency) to externalize wrapper packages when an installed optional dep is native. - Extend resolver fixture set with a “native optional wrapper” and a “plain JS optional wrapper” to cover both positive and negative cases.
- Add Vitest assertions validating the new externalization behavior.
Reviewed changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Adds #hasNativeOptionalDependency check into native externalization decision path. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Adds test coverage for native optional-wrapper externalization and JS-only optional dependency non-externalization. |
| tools/egg-bundler/test/fixtures/externals/basic-app/package.json | Declares new fixture dependencies used by the added tests. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-wrapper/package.json | Fixture wrapper package with an optional dependency. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-platform/package.json | Fixture optional dependency package. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/native-optional-platform/addon.node | Native signal fixture (*.node) for detection. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-js-wrapper/package.json | Fixture JS-only wrapper package with optional dependency. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-js-dep/package.json | Fixture JS-only optional dependency package. |
killagu
force-pushed
the
agent/egg-dev/70acad08
branch
from
1699b14 to
3daeece
Compare
Summary
Validation
cnpmcore smoke
Summary by CodeRabbit
New Features
Tests