fix: npm protocol alias with pnp should be supported#139
Merged
Conversation
WalkthroughThe updates include bumping the Yarn package manager version from 4.9.1 to 4.9.2 in two fixture package.json files, adding new alias dependencies including "custom-minimist" in one fixture, and introducing a new test to verify the resolution of these aliases to the correct cached package paths. Additionally, internal logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as resolve_npm_protocol_alias()
participant Resolver
participant YarnCache
Test->>Resolver: Initialize with pnp fixture
Test->>Resolver: Resolve "custom-minimist"
Resolver->>YarnCache: Lookup "minimist@^1.2.8"
YarnCache-->>Resolver: Return cached path to minimist
Resolver-->>Test: Return resolved path
Test->>Test: Assert resolved path matches expected
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (2)
✨ 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 (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the handling of npm protocol aliases with Plug'n'Play and updates related tests and fixtures accordingly.
- Added a new test (resolve_npm_protocol_alias) to verify proper resolution of npm protocol aliases.
- Updated Yarn package manager versions and added a "custom-minimist" dependency for alias resolution.
- Updated both project and global fixture configurations to use Yarn 4.9.2.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/pnp.rs | Added a new test to validate npm protocol alias resolution |
| fixtures/pnp/package.json | Updated Yarn version and added the "custom-minimist" dependency to test alias support |
| fixtures/global-pnp/package.json | Updated Yarn version to ensure consistency with project configuration |
Comments suppressed due to low confidence (1)
src/tests/pnp.rs:135
- Consider adding a doc comment above the 'resolve_npm_protocol_alias' test to clarify its purpose and expected behavior, which can improve maintainability and clarity for future developers.
#[test]
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9cc7b7a in 56 seconds. Click for details.
- Reviewed
53lines of code in3files - Skipped
1files 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. fixtures/global-pnp/package.json:3
- Draft comment:
Bumps Yarn version to 4.9.2. This update appears intended for PnP support – confirm it's deliberate. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding a dependency update. This violates the rule against asking for confirmation of intention, especially related to dependencies.
2. fixtures/pnp/package.json:7
- Draft comment:
Adds the 'custom-minimist' alias using the npm protocol. This correctly maps to the 'minimist' package – ensure the alias resolution in tests stays in sync. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is informative and suggests ensuring that alias resolution in tests stays in sync. It doesn't provide a specific code suggestion or ask for a specific test to be written. It also doesn't point out a potential issue or ask for confirmation of an intentional change.
3. src/tests/pnp.rs:136
- Draft comment:
New test 'resolve_npm_protocol_alias' verifies npm protocol alias resolution. Make sure the expected cached zip file hash remains valid if cache naming changes in future. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure future compatibility, which is not specific to the current code change. It doesn't provide a specific suggestion or point out a current issue with the code.
4. src/tests/pnp.rs:139
- Draft comment:
Consider using Resolver::new(ResolveOptions::default()) instead of Resolver::default() for consistency with other tests. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_xxrjHIePRMdTGohl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fixtures/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
fixtures/global-pnp/package.json(1 hunks)fixtures/pnp/package.json(1 hunks)src/tests/pnp.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
Learnt from: JounQin
PR: unrs/unrs-resolver#136
File: src/tests/extension_alias.rs:36-46
Timestamp: 2025-06-05T18:52:30.363Z
Learning: The fixtures/yarn directory in the unrs-resolver project requires running `yarn install` to install TypeScript dependencies before tests can run, similar to other fixture directories like fixtures/pnp. The test in src/tests/extension_alias.rs depends on the installed TypeScript package providing lib/typescript.d.ts and related files.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Test (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
fixtures/global-pnp/package.json (1)
3-3: LGTM! Version synchronization maintains fixture consistency.The Yarn version bump to 4.9.2 aligns with the other fixture and ensures consistent package manager versions across test environments.
fixtures/pnp/package.json (1)
3-3: LGTM! Yarn version update supports npm protocol alias.The version bump to 4.9.2 ensures compatibility with the npm protocol alias feature being implemented.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ee31e61 in 52 seconds. Click for details.
- Reviewed
73lines of code in3files - Skipped
1files 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. src/tests/pnp.rs:90
- Draft comment:
Consider renaming 'resolve_pnp_pkg_should_failed_while_disable_pnp_mode' to 'resolve_pnp_pkg_should_fail_while_pnp_mode_disabled' for clarity and proper grammar. - 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/tests/pnp.rs:135
- Draft comment:
Add inline comments in the 'resolve_npm_protocol_alias' test to clarify that it verifies resolution of npm protocol alias entries for both namespaced and non-namespaced package identifiers. - 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/tests/pnp.rs:168
- Draft comment:
Using 'dirs::home_dir().unwrap()' may panic if the home directory isn’t available; consider handling the None case or documenting that the test environment must provide a valid home directory. - 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/pnp.rs:13
- Draft comment:
Consider using a consistent method for constructing Resolver (e.g. always using Resolver::new with explicit options) across tests to improve clarity. - 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_hxotaoeC4i8H5fnw
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #139 will not alter performanceComparing Summary
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix un-ts/eslint-plugin-import-x#379
Important
Fix npm protocol alias resolution in
ResolverGenericand update Yarn version to 4.9.2 with new test added.ResolverGenericinlib.rsby using the specifier as the source of truth for package names.global-pnp/package.jsonandpnp/package.json.pnp/package.json.resolve_npm_protocol_alias()inpnp.rsto test npm protocol alias resolution.This description was created by
for ee31e61. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Chores
Tests