fix: correctly handle native package dependencies size and counts - #3111
fix: correctly handle native package dependencies size and counts#3111jibin7jose wants to merge 4 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
WalkthroughNative status now propagates through dependency resolution. Optional native packages remain visible in dependency lists but are excluded from package, size, and count totals. Tests cover status propagation and install-size filtering. ChangesNative 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/server/utils/install-size.spec.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a type-safe stub signature.
Functionerases the parameter and return types fordefineCachedFunction. Use a generic callback type so TypeScript checks the stub contract.Proposed fix
-vi.stubGlobal('defineCachedFunction', (fn: Function) => fn) +vi.stubGlobal( + 'defineCachedFunction', + <TArgs extends unknown[], TResult>(fn: (...args: TArgs) => TResult) => fn, +)As per coding guidelines, “Ensure you write strictly type-safe code”.
🤖 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 `@test/unit/server/utils/install-size.spec.ts` at line 3, Update the defineCachedFunction global stub to use a generic, type-preserving callback signature instead of Function, ensuring the stub retains the wrapped function’s parameter and return types while satisfying TypeScript’s contract checks.Source: Coding guidelines
🤖 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 `@test/unit/server/utils/install-size.spec.ts`:
- Around line 42-43: Extend the assertions for the result produced by the
install-size utility to verify each dependency’s native metadata, especially
that optional and isNative values are preserved through mapping. Keep the
existing dependencies length assertion, and add expectations against the
returned dependency entries rather than only checking the collection size.
---
Nitpick comments:
In `@test/unit/server/utils/install-size.spec.ts`:
- Line 3: Update the defineCachedFunction global stub to use a generic,
type-preserving callback signature instead of Function, ensuring the stub
retains the wrapped function’s parameter and return types while satisfying
TypeScript’s contract checks.
🪄 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: d1de03c4-edac-43be-91c4-764576afb136
📒 Files selected for processing (2)
test/unit/server/utils/dependency-resolver.spec.tstest/unit/server/utils/install-size.spec.ts
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.