fix: should try package exports first per spec #118
Conversation
WalkthroughThe update refines the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResolverGeneric
Caller->>ResolverGeneric: load_node_modules(specifier)
alt Cached path is file and alias_fields empty
ResolverGeneric-->>Caller: Return cached file path
else Specifier does not end with slash
ResolverGeneric->>ResolverGeneric: load_as_file()
alt File found
ResolverGeneric-->>Caller: Return file path
else
alt Cached path is directory
ResolverGeneric->>ResolverGeneric: load_browser_field_aliases_or_directory_index()
alt Success
ResolverGeneric-->>Caller: Return directory result
else
ResolverGeneric-->>Caller: Return error or fallback
end
else
ResolverGeneric-->>Caller: Return error or fallback
end
end
else
alt Cached path is directory
ResolverGeneric->>ResolverGeneric: load_browser_field_aliases_or_directory_index()
ResolverGeneric-->>Caller: Return directory result
else
ResolverGeneric-->>Caller: Return error or fallback
end
end
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
=======================================
Coverage 93.42% 93.43%
=======================================
Files 13 13
Lines 2876 2879 +3
=======================================
+ Hits 2687 2690 +3
Misses 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 0bcdede in 1 minute and 13 seconds. Click for details.
- Reviewed
50lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib.rs:778
- Draft comment:
Ensure that package exports are prioritized per spec. Here the code calls load_package_exports before falling back to file/directory lookup—confirm this ordering matches Node.js behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/lib.rs:833
- Draft comment:
There is a repeated call to load_as_directory on the same cached_path. Consider refactoring to avoid duplicate filesystem checks. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/lib.rs:797
- Draft comment:
The fallback logic for scoped packages (names beginning with '@') uses a parent's directory check. It would help to add a clarifying comment explaining the intent of this check. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_n9yHdRffO2RKAPIK
You can customize 
There was a problem hiding this comment.
Pull Request Overview
This PR updates the module resolution order to match the Node.js spec by trying package exports first, then falling back to file and directory resolution.
- Removes the old early
load_as_fileblock for bare specifiers - Inserts a unified check that first returns direct files, then calls
load_as_file(if the specifier doesn’t end with a slash), before directory resolution
Comments suppressed due to low confidence (2)
src/lib.rs:820
- [nitpick] The variable name
no_slashis ambiguous. Consider renaming it to something likeis_bare_specifierorhas_no_path_segmentsfor clarity.
let no_slash = !specifier.contains('/');
CodSpeed Performance ReportMerging #118 will not alter performanceComparing Summary
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Benchmark
- GitHub Check: Test (windows-latest)
🔇 Additional comments (2)
src/lib.rs (2)
825-831: Good implementation of Node.js specification compliance.This conditional logic correctly implements the Node.js module resolution algorithm by trying file resolution before directory resolution, and the condition
no_slash || !specifier.ends_with('/')appropriately handles different specifier types.
786-791: Excellent alignment with Node.js specification order.The overall flow now correctly follows the Node.js module resolution algorithm:
- Package exports first (lines 786-791)
- File resolution second (lines 827-831)
- Directory resolution last (lines 833-840)
This change brings the resolver in line with the official Node.js specification as intended.
Also applies to: 827-831, 833-840
JounQin
force-pushed
the
fix/try_exports_first
branch
from
0bcdede to
bb2412c
Compare
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bb2412c in 1 minute and 1 seconds. Click for details.
- Reviewed
92lines of code in4files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. fixtures/prefer-file-over-dir/package.json:3
- Draft comment:
Consider adding a trailing newline at the end of the file for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/tests/resolve.rs:39
- Draft comment:
Duplicate test entry for 'file in module with query and fragment' detected; consider removing one occurrence to avoid redundancy. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_zt8FvjADuprJkyR3
You can customize
There was a problem hiding this comment.
Pull Request Overview
This PR reworks module resolution logic to prioritize package exports over file and directory resolution per the Node.js spec. It adjusts the resolution order in ResolverGeneric::load_node_modules, renames the test from "prefer_file_over_pkg" to "prefer_file_over_dir" (with corresponding fixture updates), and enhances test coverage for both unscoped and scoped package names.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib.rs | Updated resolution order to check load_as_file before load_as_directory when the specifier does not end with a slash. |
| src/tests/resolve.rs | Renamed test function and updated test cases to reflect new resolution behavior. |
| fixtures/prefer-file-over-dir | Updated fixture naming to align with test expectations. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
fixtures/prefer-file-over-dir/node_modules/@foo/bar.jsis excluded by!**/node_modules/**fixtures/prefer-file-over-dir/node_modules/@foo/bar/index.jsis excluded by!**/node_modules/**fixtures/prefer-file-over-dir/node_modules/bar.jsis excluded by!**/node_modules/**fixtures/prefer-file-over-dir/node_modules/bar/index.jsis excluded by!**/node_modules/**fixtures/prefer-file-over-dir/node_modules/bar/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (4)
fixtures/prefer-file-over-dir/package.json(1 hunks)fixtures/prefer-file-over-pkg/package.json(0 hunks)src/lib.rs(2 hunks)src/tests/resolve.rs(1 hunks)
💤 Files with no reviewable changes (1)
- fixtures/prefer-file-over-pkg/package.json
✅ Files skipped from review due to trivial changes (1)
- fixtures/prefer-file-over-dir/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/tests/resolve.rs (2)
122-122: Good naming improvement for test clarity.The function name change from
prefer_file_over_pkgtoprefer_file_over_dirbetter reflects the actual behavior being tested - that files are preferred over directories in module resolution, which aligns with the Node.js specification changes mentioned in the PR.
123-123: Fixture path updated consistently.The fixture directory path change maintains consistency with the renamed test function, which is good practice for test organization and maintainability.
rework #116
https://nodejs.org/api/modules.html#all-together
Important
Fix module resolution to prioritize package exports over files and directories, aligning with Node.js spec.
ResolverGeneric::load_node_modulesinsrc/lib.rsto prioritizeload_as_filebeforeload_as_directorywhenspecifierdoes not end with a slash.prefer_file_over_pkgtest toprefer_file_over_dirinsrc/tests/resolve.rsto reflect new resolution order.prefer-file-over-pkgtoprefer-file-over-dirinfixturesto align with updated test cases.src/tests/resolve.rsto verify new resolution order for package exports over files and directories.This description was created by
for bb2412c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit