fix: correctly handle native package dependencies size and counts - #3111
fix: correctly handle native package dependencies size and counts#3111jibin7jose wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @jibin7jose! 🚀 Here’s what will happen next:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesNative optional dependency handling
Sequence Diagram(s)sequenceDiagram
participant dependency_resolver
participant dependency_analysis
participant install_size
dependency_resolver->>dependency_analysis: Provide resolved packages with native status
dependency_resolver->>install_size: Provide resolved packages with native status
dependency_analysis->>dependency_analysis: Exclude native optional packages from totals
install_size->>install_size: Exclude native optional packages from size and count
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/dependency-resolver.ts (1)
203-223: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve required-path reachability when paths converge.
Line 203 discards a later path when
nextLevelalready containsdepName. If an optional native branch inserts the package first, a required non-native branch is ignored. Concurrent traversal makes the retainedoptionalandisNativeParentstate order-dependent.The downstream aggregate filters then exclude a package that a required dependency path installs. Track traversal state per path, or merge states so exclusion applies only when no required non-native path reaches the package. Reprocess descendants when merged state changes.
🤖 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 `@server/utils/dependency-resolver.ts` around lines 203 - 223, The dependency traversal around the `nextLevel.set` calls must merge converging paths instead of discarding later entries by `depName`. Ensure a required non-native path overrides optional/native-only state, preserve the package when any such path reaches it, and reprocess descendants whenever the merged `optional` or `isNativeParent` state changes; update the downstream aggregate filtering accordingly.
🤖 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 `@server/utils/dependency-resolver.ts`:
- Around line 164-188: Update the libc detection in the selfIsNative calculation
to read through a typed local value instead of casting versionData to any.
Preserve the existing Array.isArray and non-empty checks, matching the type-safe
handling of os and cpu.
---
Outside diff comments:
In `@server/utils/dependency-resolver.ts`:
- Around line 203-223: The dependency traversal around the `nextLevel.set` calls
must merge converging paths instead of discarding later entries by `depName`.
Ensure a required non-native path overrides optional/native-only state, preserve
the package when any such path reaches it, and reprocess descendants whenever
the merged `optional` or `isNativeParent` state changes; update the downstream
aggregate filtering accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 131d3e8f-8906-44ea-8ce5-871571cb4a2b
📒 Files selected for processing (4)
server/utils/dependency-analysis.tsserver/utils/dependency-resolver.tsserver/utils/install-size.tsshared/types/install-size.ts
gameroman
left a comment
gameroman
left a comment
There was a problem hiding this comment.
Please add tests and follow the PR template
ok |
2 participants
Resolves #3110
Description
This PR addresses an issue where optional dependencies for native packages (specifically WASM bindings, like
@rolldown/binding-wasm32-wasi) were being incorrectly tracked as regular dependencies. This resulted in massively inflated install size calculations and incorrect total transitive dependency counts (e.g.,rolldownreporting 10 total dependencies and +14 MB in size instead of 2 total dependencies).Changes Made
server/utils/dependency-resolver.ts):-wasm32-,-wasm32, orwasm32-wasi) to correctly categorize them as native packages.isNativeflag correctly trickles down and is inherited by all transitive children of a native package.server/utils/install-size.ts):server/utils/dependency-analysis.ts):isNativeflag now propagates correctly, transitive children of native packages are effectively filtered out of thetotalPackagescount.Testing
rolldownnow correctly reports2total dependencies and an install size of~895 KB.