fix: support resolve from scoped package parent node_modules #36
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new error handling mechanism for import resolution has been introduced. The changes include creating an Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ImportResolver
participant ModuleResolver
participant ErrorHandler
Caller->>ImportResolver: Request module import
ImportResolver->>ModuleResolver: Attempt to resolve path
alt Path resolution successful
ModuleResolver-->>ImportResolver: Return resolved path
ImportResolver-->>Caller: Return imported module
else Path resolution fails
ModuleResolver->>ErrorHandler: Create ImportResolveError
ErrorHandler-->>ImportResolver: Throw detailed error
ImportResolver->>Caller: Throw ImportResolveError
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #36 +/- ##
==========================================
+ Coverage 87.50% 88.25% +0.75%
==========================================
Files 6 8 +2
Lines 696 741 +45
Branches 120 130 +10
==========================================
+ Hits 609 654 +45
Misses 87 87 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/import.ts (2)
208-239: Consider deeper or repeated parent traversal.Currently, the code checks two levels of parent
node_modulesdirectories for scoped and non-scoped packages. If a library or framework requires deeper nesting, a loop might be needed. Otherwise, this approach is sufficient for typical use cases.
249-255: Use consistent error messages for better user experience.The test validates error messages against
/Cannot find package/. This depends on the underlying system error text fromimport.meta.resolve. To avoid fragile tests in future Node versions, consider providing a clearer, custom prefix or clarifying text in the final error message instead of relying on the underlying message alone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/fixtures/cjs/node_modules/@foo/bar/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (5)
src/error/ImportResolveError.ts(1 hunks)src/error/index.ts(1 hunks)src/import.ts(3 hunks)src/index.ts(1 hunks)test/import.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/error/index.ts
🔇 Additional comments (6)
src/error/ImportResolveError.ts (2)
1-3: Robust custom error implementation.This class definition is well thought out, with typed fields for
filepathandpaths, ensuring that relevant context is captured whenever an import resolve error occurs.
5-7: Verify Node.js compatibility for thecauseoption.Passing the original error as
{ cause: error }is a good approach for debugging, but be aware that this feature was introduced in Node.js 16.9.0. If this library must support earlier Node versions, consider a fallback strategy.✅ Verification successful
Based on the shell script results, I can see that the package.json specifies Node.js version requirement as:
"engines": { "node": ">=18.19.0" }The
causeoption in Error constructor is fully compatible with the codebase's Node.js requirements.The Error constructor's
causeoption was introduced in Node.js 16.9.0, and this codebase requires Node.js 18.19.0 or higher, which means the feature is fully supported in all supported Node.js versions for this library. No fallback strategy is needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for minimum required Node version or engine constraints in the repo. rg "engines" rg "node" -A 5Length of output: 27446
src/index.ts (1)
12-12: Good re-export for consolidated error handling.Re-exporting artifacts from
./error/index.jsis a convenient way to maintain a flat API surface, especially for consumers who only want to import fromsrc/index.ts.test/import.test.ts (3)
3-3: Clear usage of ImportResolveError.Adding
ImportResolveErrorto the test scope demonstrates robust coverage for the new error handling logic. Good job integrating it into existing tests.
40-48: Sufficient verification of parent node_modules resolution.This test confirms the new fallback logic for searching parent
node_modules. The approach is straightforward and ensures that resolution logic meets user expectations.
50-63: Avoid matching system error messages to ensure stable tests.Matching
err.messagewith/Cannot find package/could break if Node error strings change. For consistent test outcomes, consider including a stable prefix in your thrown error message (e.g.,"ImportResolveError: Cannot find package").
[skip ci] ## [4.2.3](v4.2.2...v4.2.3) (2025-01-03) ### Bug Fixes * support resolve from scoped package parent node_modules ([#36](#36)) ([7c9cc42](7c9cc42))
Summary by CodeRabbit
New Features
ImportResolveErrorclass for more detailed import resolution error handlingnode_modulesdirectoriesBug Fixes
Tests
ImportResolveErrorand expanded module resolution testingDocumentation