fix: honor the root boundary and fall back to index for broken entry fields - #14
fix: honor the root boundary and fall back to index for broken entry fields#14DylanPiercey wants to merge 1 commit into
Conversation
…fields The node_modules/imports walk was a do/while that tested `dir !== root` only after moving to the parent, so it stopped one directory short of `root` whenever `from` was nested below it. As a result `<root>/node_modules` and a `package.json` at `<root>` (for `#imports`) were never searched — the documented case of pointing `root` at a project directory. The walk now runs until it has examined `root` itself and stops above it, builds child paths without a doubled slash at a file system root, and normalizes a trailing slash on `root`. Separately, a package/directory whose entry field (e.g. `main`) points at a missing file now falls through to a later field and finally to `index`, matching Node's LOAD_AS_DIRECTORY and the resolve/webpack resolvers, instead of throwing when an index file is present.
🦋 Changeset detectedLatest commit: 5c711aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 97.01% 97.24% +0.22%
==========================================
Files 1 1
Lines 402 435 +33
Branches 87 96 +9
==========================================
+ Hits 390 423 +33
Misses 11 11
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WalkthroughUpdated package resolution so missing entry targets can fall through to later fields and ultimately an index file. Revised directory walking to normalize roots, include the configured root in searches, and avoid doubled-slash paths. Added tests for package fallbacks, dependency boundaries, trailing-root normalization, and imports resolution. Updated changesets document the behavior changes and patch releases. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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)
src/__tests__/index.test.ts (1)
797-880: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a filesystem-root regression test.
The changeset promises avoiding probes such as
//node_modules, but these tests only exercise/projectroots. Add a case usingroot: "/"(or the default root) with a dependency at/node_modules/...and assert the resolved path.🤖 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 `@src/__tests__/index.test.ts` around lines 797 - 880, Extend the “resolve - root boundary” tests with a filesystem-root case using root "/" (or the default root), placing the package under /node_modules with its package.json and entry file, and assert resolveSync returns the package entry path without probing a double-slash node_modules location.
🤖 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 `@src/index.ts`:
- Line 100: Update normalizeRoot and its usage in the root-boundary resolution
logic to remove all repeated trailing separators, while preserving the
filesystem root and drive roots. Ensure normalized configured roots match
directory-walking results and cannot resolve above the configured root, then add
a regression test covering multiple trailing slashes.
---
Nitpick comments:
In `@src/__tests__/index.test.ts`:
- Around line 797-880: Extend the “resolve - root boundary” tests with a
filesystem-root case using root "/" (or the default root), placing the package
under /node_modules with its package.json and entry file, and assert resolveSync
returns the package entry path without probing a double-slash node_modules
location.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7ef3429-32aa-4256-87f7-5da9041f090e
📒 Files selected for processing (4)
.changeset/main-index-fallback.md.changeset/root-boundary-inclusive.mdsrc/__tests__/index.test.tssrc/index.ts
| const fs = opts.fs || defaultFS; | ||
| const realpath = (!opts.preserveSymlinks && fs.realpath) || identity; | ||
| const root = toPosix(opts.root || "/"); | ||
| const root = normalizeRoot(toPosix(opts.root || "/")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize all trailing separators, not just one.
normalizeRoot("/project//") becomes "/project/", while directory walking produces "/project". The boundary comparison then never matches, so resolution can continue to /node_modules above the configured root. Trim repeated trailing / characters while preserving / and drive roots, and add a regression test for multiple trailing slashes.
Proposed fix
function normalizeRoot(root: string) {
- const end = root.length - 1;
+ let end = root.length;
+ while (
+ end > 1 &&
+ root.charCodeAt(end - 1) === 47 &&
+ !(end === 3 && root.charCodeAt(1) === 58)
+ ) {
+ end--;
+ }
- return end > 0 &&
- root.charCodeAt(end) === 47 &&
- !(end === 2 && root.charCodeAt(1) === 58)
- ? root.slice(0, end)
- : root;
+ return root.slice(0, end);
}Also applies to: 359-368
🤖 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 `@src/index.ts` at line 100, Update normalizeRoot and its usage in the
root-boundary resolution logic to remove all repeated trailing separators, while
preserving the filesystem root and drive roots. Ensure normalized configured
roots match directory-walking results and cannot resolve above the configured
root, then add a regression test covering multiple trailing slashes.
1 participant
Description
Two resolution correctness fixes:
1. The
rootoption no longer excludes the root directory itself. Thenode_modules/importswalk was ado…while (dir !== root)that only tested the condition after stepping to the parent, so it stopped one directory short ofrootwheneverfromwas nested below it. As a result<root>/node_modules— and apackage.jsonat<root>for#imports— were never searched. That is exactly the documented case (pointingrootat a project directory whose dependencies live in<root>/node_modules), which threwCannot find module. The bug was masked in day-to-day use because the defaultrootis/, where the only unsearched directory is the near-useless/node_modules.The walk now runs until it has examined
rootitself and stops above it. As a side effect it no longer probes doubled-slash paths (e.g.//node_modules) at a file system root, and a trailing slash onrootis normalized so"/project/"behaves like"/project".2. A broken entry field falls back to
index. When a package/directory entry field (e.g.main) pointed at a file that does not exist, resolution threw even when anindexfile was present. It now falls through to a later field infieldsand finally toindex, matching Node'sLOAD_AS_DIRECTORYand theresolve/webpack resolvers. A genuinely unresolvable package (no working field and no index) still throws as before.Motivation and Context
The
rootbehavior contradicted the README, which states resolution "won't ascend beyond this directory" and "will not resolve modules outside of/project" — implying<root>/node_modulesis in scope. In practice, settingrootto a real project directory broke package and subpath-import resolution. The index-fallback change aligns with the stated goal of "resolving in the same way node and bundlers do," verified againstrequire.resolveand theresolvepackage.Both fixes are additive/relaxing (they turn former throws into successful resolutions) and keep the existing boundary and error semantics: nothing above
rootis searched, and truly missing entries still throw.Screenshots (if appropriate):
N/A
Checklist:
Generated by Claude Code