fix: remove invalid file cache due to extension alias #136
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant Resolver as Resolver
participant FileSystem as File System
TestRunner->>Resolver: resolve("typescript/lib/typescript.js", root="fixtures/yarn")
Resolver->>FileSystem: check for typescript.js
alt .js extension aliasing enabled
Resolver->>FileSystem: check for typescript.d.ts
alt typescript.d.ts exists
Resolver-->>TestRunner: return path to typescript.d.ts
else typescript.d.ts does not exist
Resolver->>FileSystem: check for typescript.ts
alt typescript.ts exists
Resolver-->>TestRunner: return path to typescript.ts
else
Resolver-->>TestRunner: resolution failed
end
end
else
Resolver-->>TestRunner: return path to typescript.js or fail
end
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 (
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that extension aliases are always considered by removing a premature file-cache shortcut, and it adds a Yarn‐based fixture with tests and CI updates to validate .js→.d.ts resolution.
- Removed the early return in
lib.rssoextension_aliasis applied even when a file exists. - Introduced a new Yarn fixture under
fixtures/yarnand a corresponding test for.js→.d.ts. - Updated build scripts (
justfileand GitHub Actions) to install the new fixture.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Removed shortcut is_file check to allow extension_alias logic |
| src/tests/extension_alias.rs | Added test for .js → .d.ts resolution with multiple aliases |
| justfile | Added cd fixtures/yarn && yarn to the install recipe |
| fixtures/yarn/package.json | New fixture’s package.json |
| fixtures/yarn/.yarnrc.yml | Yarn config for the fixture |
| fixtures/yarn/.gitignore | Gitignore for the fixture |
| .github/actions/pnpm/action.yml | CI updated to run yarn install in fixtures/yarn |
Comments suppressed due to low confidence (2)
src/lib.rs:843
- Consider restoring a fast path that skips
load_as_fileonly when neitheralias_fieldsnorextension_aliasis configured, to avoid extra filesystem checks when no aliases are in use.
if self.options.alias_fields.is_empty() && self.cache.is_file(&cached_path, ctx) {
src/tests/extension_alias.rs:38
- [nitpick] Consider adding a test case where a
.jsfile resolves to.tswhen it exists, to verify that the first alias in the list is preferred.
extension_alias: vec![(".js".into(), vec![".ts".into(), ".d.ts".into()])],
![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 2744c26 in 1 minute and 12 seconds. Click for details.
- Reviewed
89lines of code in7files - Skipped
2files when reviewing. - Skipped posting
4draft 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. .github/actions/pnpm/action.yml:37
- Draft comment:
New yarn install steps for fixtures/global-pnp and fixtures/yarn added. Ensure these directories exist in all environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that certain directories exist in all environments. This falls under the rule of not asking the author to ensure something is tested or verified, which is not allowed.
2. justfile:15
- Draft comment:
Added 'cd fixtures/yarn && yarn' in install target. Verify it aligns with your project setup. - 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.
3. src/lib.rs:315
- Draft comment:
Removed early return caching logic for alias_fields; ensure this fix correctly prevents caching of invalid files without impacting performance on resolutions without aliasing. - 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.
4. src/tests/extension_alias.rs:20
- Draft comment:
Extension alias tests look comprehensive. They verify both successful alias resolution and proper error when fallback isn’t allowed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_v1i7WVm7QXsFMyzG
You can customize 
![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 (2)
fixtures/yarn/.gitignore (1)
1-2: Unignore release files properly
The pattern!.yarn/releasesre-includes the directory but not its contents. To ensure the release binaries (e.g.,.cjsfiles) are tracked, update to:!.yarn/releases/ !.yarn/releases/*.cjsfixtures/yarn/package.json (1)
1-7: Add version and private fields
Yarn expects aversionfield and adding"private": trueprevents accidental publishes. Consider:{ "name": "extension-alias", + "version": "0.0.0", + "private": true, "packageManager": "[email protected]", "devDependencies": { "typescript": "^5.8.3" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fixtures/yarn/.yarn/releases/yarn-4.9.2.cjsis excluded by!**/.yarn/**fixtures/yarn/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.github/actions/pnpm/action.yml(1 hunks)fixtures/yarn/.gitignore(1 hunks)fixtures/yarn/.yarnrc.yml(1 hunks)fixtures/yarn/package.json(1 hunks)justfile(1 hunks)src/lib.rs(0 hunks)src/tests/extension_alias.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/lib.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
justfile (2)
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
.github/actions/pnpm/action.yml (2)
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
🧬 Code Graph Analysis (1)
src/tests/extension_alias.rs (2)
src/options.rs (1)
default(476-502)src/tests/mod.rs (1)
fixture_root(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
justfile (1)
19-19: Align with fixture installation pattern
Addingcd fixtures/yarn && yarnmirrors the existing steps for other fixtures and ensures local environments are correctly prepared..github/actions/pnpm/action.yml (1)
39-41: Include Yarn fixture in CI installs
The newcd ../yarn && yarn installstep ensures that thefixtures/yarndependencies are installed during CI, aligning with the localjustfile.fixtures/yarn/.yarnrc.yml (1)
1-4: Pin Yarn and enforce node-modules linker
The.yarnrc.ymlcorrectly configuresnodeLinker: node-modulesand pointsyarnPathto the pinned release.yarn/releases/yarn-4.9.2.cjs, guaranteeing consistent installs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
- Coverage 93.40% 93.39% -0.01%
==========================================
Files 13 13
Lines 2894 2890 -4
==========================================
- Hits 2703 2699 -4
Misses 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #136 will not alter performanceComparing Summary
|
Important
Fix file caching issue by removing incorrect condition in
ResolverGeneric::requireand add test for extension aliasing.ResolverGeneric::requireinsrc/lib.rsthat returned cached paths without considering extension aliases.extension_alias.rsto verify correct resolution of.jsto.tsand.d.tsusing extension aliases.yarnfixture with.gitignore,.yarnrc.yml, andpackage.jsonfor testing purposes.action.ymlandjustfileto includeyarninstallation steps.This description was created by
for 2744c26. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
.jsto.tsand.d.tsfiles in the new Yarn fixture.