fix: use importResolve to get framework path #31
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant importResolve
participant FileSystem
participant DebugLogger
Caller->>importResolve: Resolve module path
importResolve->>DebugLogger: Log resolution attempt
importResolve->>FileSystem: Check module file status
alt Module file exists
FileSystem-->>importResolve: File found
importResolve-->>Caller: Return resolved path
else Module file not found
FileSystem-->>importResolve: No file
importResolve->>DebugLogger: Log error details
importResolve->>Caller: Throw TypeError
end
Possibly Related PRs
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: |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31 +/- ##
==========================================
+ Coverage 88.34% 89.44% +1.10%
==========================================
Files 6 6
Lines 609 616 +7
Branches 100 104 +4
==========================================
+ Hits 538 551 +13
+ Misses 71 65 -6 ☔ 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: 1
🧹 Nitpick comments (1)
test/import.test.ts (1)
14-18: Enhance test coverage for negative scenarios
This approach thoroughly tests a CommonJS module undercoffee.fork. Consider adding a test case that verifies behavior when the module fails or is missing to improve robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
package.json(1 hunks)src/framework.ts(2 hunks)src/import.ts(1 hunks)test/fixtures/cjs-index/index.cjs(1 hunks)test/fixtures/cjs-index/package.json(1 hunks)test/fixtures/cjs/run.js(1 hunks)test/fixtures/egg-app/config/plugin.test.js(0 hunks)test/fixtures/egg-app/get_loadunit.js(1 hunks)test/fixtures/egg-app/package.json(1 hunks)test/fixtures/esm-index/index.mjs(1 hunks)test/fixtures/esm-index/package.json(1 hunks)test/import.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/egg-app/config/plugin.test.js
✅ Files skipped from review due to trivial changes (4)
- test/fixtures/cjs-index/package.json
- test/fixtures/esm-index/package.json
- test/fixtures/esm-index/index.mjs
- test/fixtures/egg-app/get_loadunit.js
🔇 Additional comments (13)
test/fixtures/egg-app/package.json (1)
5-5: LGTM! Verify beta tag stability.
The change to use the "beta" tag for @eggjs/core aligns with the egg dependency version and is appropriate for test fixtures. However, ensure that the beta versions are stable enough for testing purposes.
Let's verify the beta tags:
✅ Verification successful
Beta tags are stable and aligned with major version transitions
The beta versions are stable development releases:
- @eggjs/core beta (6.3.0-beta.0) is a minor version bump from the latest stable (6.2.4)
- egg beta (4.0.0-beta.7) represents the upcoming v4 major release with multiple beta iterations
Using these beta versions in test fixtures is appropriate as they:
- Follow semantic versioning conventions
- Are official pre-releases from the maintainers
- Have consistent version alignment between dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the available versions and beta tags for @eggjs/core and egg
# Check available versions for @eggjs/core
echo "Available versions for @eggjs/core:"
curl -s https://registry.npmjs.org/@eggjs/core | jq -r '.["dist-tags"]'
# Check available versions for egg
echo -e "\nAvailable versions for egg:"
curl -s https://registry.npmjs.org/egg | jq -r '.["dist-tags"]'
Length of output: 487
test/fixtures/cjs-index/index.cjs (2)
1-3: Looks good for CommonJS exports.
Exporting an object with a foo: 'bar' property is straightforward and aligns with CommonJS usage.
5-5: Adding extra property on module.exports.
Appending one = 1 directly to module.exports is a clean approach.
test/fixtures/cjs/run.js (2)
1-1: Verify the relative import path.
Using require('../../../') might break if the directory structure changes. Confirm that this relative path is correct and robust in the broader project context.
3-5: Debug log usage.
Logging the result of importResolve using %o is suitable for object debugging. No issues found.
src/framework.ts (4)
1-1: Use of debuglog for targeted debugging.
Importing debuglog from node:util is a well-structured approach to conditionally enable verbose logs.
6-7: Integration of importResolve.
Replacing require.resolve with the more flexible importResolve is a solid improvement for advanced module resolution scenarios.
73-78: Handling scoped and unscoped packages differently.
Using importResolve(${frameworkName}/package.json) and adjusting how far to walk up the directory tree constitutes a targeted approach to handle scoping. Double-check that no off-by-one directory jumps occur for complex package paths.
85-85: Robust error logging.
Using debug to log errors when importResolve fails helps with diagnosing resolution issues. This is a best practice for maintainability.
src/import.ts (1)
191-193: Enhanced error checking for resolved files.
Using fs.statSync(..., { throwIfNoEntry: false }) combined with an explicit file check (.isFile()) provides robust validation. Throwing a TypeError prevents silent failures. Nicely done.
test/import.test.ts (3)
2-2: Confirm that 'coffee' is properly declared as a dependency
Make sure 'coffee' is added to the project's devDependencies or dependencies to avoid runtime or testing issues.
11-12: Good test coverage for .cjs resolution
Verifies that cjs-index resolves correctly to cjs-index/index.cjs.
23-23: Sufficient clarity for esm-index resolution
This assertion ensures that esm-index resolves to index.mjs, reflecting accurate coverage of ESM module resolution.
[skip ci] ## [4.1.5](v4.1.4...v4.1.5) (2024-12-27) ### Bug Fixes * use importResolve to get framework path ([#31](#31)) ([7caad4d](7caad4d))
Summary by CodeRabbit
New Features
Bug Fixes
importResolvefunction to provide more context during module resolution failures.Documentation
Chores
package.jsonfiles for various modules.