fix: custom condition_names should take higher priority than target in package.json
#115
Conversation
WalkthroughA new fixture named Changes
Sequence Diagram(s)sequenceDiagram
participant Test as dual_condition_names() Test
participant Resolver as Resolver
participant FS as Filesystem
Test->>Resolver: Create with conditions ["import", "require"]
Test->>Resolver: resolve("zod", fixture_dir)
Resolver->>Resolver: Add "default" to conditions if missing
Resolver->>FS: Look up "zod" in fixture_dir with conditions
FS-->>Resolver: Return resolved path to zod/index.mjs
Resolver-->>Test: Return resolved path
Test->>Test: Assert path matches expected
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
✨ 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 fixes an issue where custom condition names were not resolved correctly by adding a dedicated test, updating the pnpm workspace configuration, and providing a corresponding package fixture.
- Added a new test (dual_condition_names) in resolve_test.rs to verify resolution with dual condition names.
- Updated pnpm-workspace.yaml to include the new fixture.
- Created a package.json fixture for dual condition names.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/resolve_test.rs | Added test dual_condition_names to verify resolution behavior |
| pnpm-workspace.yaml | Included the dual-condition-names fixture in the workspace list |
| fixtures/dual-condition-names/package.json | Added package.json file for the new fixture |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to a203a53 in 31 seconds. Click for details.
- Reviewed
46lines of code in3files - Skipped
1files 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. fixtures/dual-condition-names/package.json:1
- Draft comment:
Fixture looks good; ensure the zod version (~3.24.4) matches expected resolution. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. pnpm-workspace.yaml:4
- Draft comment:
Added the dual-condition-names fixture to the workspace; YAML structure is correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/resolve_test.rs:251
- Draft comment:
New test 'dual_condition_names' correctly verifies resolution using custom condition_names. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_6BoES77MkFeEm1gG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
a203a53 to
9db41ed
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9db41ed in 50 seconds. Click for details.
- Reviewed
46lines of code in3files - Skipped
1files 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. fixtures/dual-condition-names/package.json:1
- Draft comment:
New test fixture with dependency on 'zod'; the JSON is correctly structured. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. pnpm-workspace.yaml:4
- Draft comment:
Added new fixture 'fixtures/dual-condition-names' to the workspace; configuration looks proper. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/resolve_test.rs:251
- Draft comment:
Test 'dual_condition_names' correctly verifies resolution with custom condition_names; expected resolution to index.mjs is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_plgRDDu0GZiRVdtn
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
fixtures/dual-condition-names/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)tests/resolve_test.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pnpm-workspace.yaml (1)
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: CI
tests/resolve_test.rs
[error] 259-259: Test 'dual_condition_names' failed due to assertion failure: expected Ok(".../index.mjs") but got Ok(".../index.js").
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
fixtures/dual-condition-names/package.json (1)
1-6: LGTM! Well-structured test fixture.The fixture package configuration is clean and appropriate for testing dual condition name resolution with the zod dependency.
pnpm-workspace.yaml (1)
4-4: LGTM! Correctly integrates the new fixture into the workspace.The addition follows the existing pattern and properly includes the dual-condition-names fixture in the pnpm workspace.
condition_names specifiedcondition_names should take higher priority than target in package.json
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1956700 in 1 minute and 51 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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:1769
- Draft comment:
Bug: The iterator overcondition_namesis consumed by the.any()call. If "default" is present, the iterator is already exhausted and the subsequentforloop won’t iterate over any conditions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is correct that .any() consumes the iterator. However, the code explicitly creates a new iterator on line 1774 with conditions_vec.iter() after modifying the vector. This is intentional - the iterator is meant to be consumed by .any() and then recreated. The code is actually handling this case correctly. Could I be missing some subtle case where recreating the iterator doesn't work? Or some performance implication? No - the code explicitly creates a new iterator after the .any() call, so there is no bug. While using .contains() might be slightly more idiomatic, the current code is correct. The comment should be deleted because it incorrectly identifies a bug - the code is actually handling the iterator consumption and recreation correctly.
Workflow ID: wflow_nEFaD2bwNHdH2CL3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
CodSpeed Performance ReportMerging #115 will not alter performanceComparing Summary
|
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(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Test (windows-latest)
- GitHub Check: Benchmark
🔇 Additional comments (1)
src/lib.rs (1)
1780-1780: 🛠️ Refactor suggestionUpdate condition comparison for borrowed string.
Since we're now iterating over
&Stringinstead ofString, the comparison needs to be adjusted.Apply this diff:
- if let Some((_, target_value)) = target.iter().find(|(key, _)| condition == key) { + if let Some((_, target_value)) = target.iter().find(|(key, _)| condition == key) {Actually, this line should work as-is since string comparison handles the reference automatically. However, for clarity, you might want to make it explicit:
- if let Some((_, target_value)) = target.iter().find(|(key, _)| condition == key) { + if let Some((_, target_value)) = target.iter().find(|(key, _)| condition.as_str() == key) {Likely an incorrect or invalid review comment.
… in package.json
1956700 to
6e0f63a
Compare
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6e0f63a in 43 seconds. Click for details.
- Reviewed
77lines of code in3files - Skipped
0files 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/imports_field.rs:1
- Draft comment:
The test file covers many edge cases for imports field resolution. Consider splitting the very long list of test cases into smaller, grouped modules or a table‐driven test harness to improve readability and maintainability. - 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/imports_field.rs:108
- Draft comment:
The helper functionimports_fielduses Box::leak to extend lifetimes. This is acceptable in tests but document that this is only for testing purposes. - 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/imports_field.rs:121
- Draft comment:
Several test cases use duplicated names (e.g. multiple 'sample #1'). Unique test case identifiers would aid in diagnosing failures. - 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/imports_field.rs:60
- Draft comment:
In tests like 'test_simple()' and 'shared_resolvers()', comparisons using assert_eq! for resolution errors rely on PartialEq for error types. Verify that custom error types implement PartialEq consistently to avoid brittle test assertions. - 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_kwbqgvOqXdU41JTM
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@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 93.40% 93.41% +0.01%
==========================================
Files 13 13
Lines 2865 2870 +5
==========================================
+ Hits 2676 2681 +5
Misses 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
related un-ts/eslint-plugin-import-x#272 (comment)
Important
Custom
condition_namesnow take precedence overdefaultin module resolution, with new tests and fixtures added.condition_namesnow take precedence overdefaultinsrc/lib.rs.defaultas a fallback condition if not present.dual-condition-names/package.jsonwith dependencyzod.pnpm-workspace.yamlto includefixtures/dual-condition-names.dual_condition_names()test inresolve_test.rsto verify module resolution with multiple condition names.exports_field.rsandimports_field.rsto reflect new condition name precedence.This description was created by
for 6e0f63a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit