Revert "fix: custom condition_names should take higher priority than target in package.json (#115)"

[JounQin]

This reverts commit 1e51e82.

Spec: https://nodejs.org/api/packages.html#conditional-exports

Test multiple --condition flags: JounQin/test#4

---

Important

Reverts prioritization of custom condition_names over target in package.json, adjusting resolution logic and test expectations accordingly.

• Behavior:
  • Reverts prioritization of custom condition_names over target in package.json in src/lib.rs.
  • Removes logic that added default condition to conditions list in src/lib.rs.
• Tests:
  • Adjusts expected paths in test_cases() in exports_field.rs and imports_field.rs to reflect reverted behavior.
  • Removes dual_condition_names test in resolve_test.rs.
• Misc:
  • Deletes fixtures/dual-condition-names/package.json.
  • Removes fixtures/dual-condition-names from pnpm-workspace.yaml.

^{This description was created by for b56962d. You can customize this summary. It will automatically update as commits are pushed.}

Summary by CodeRabbit

• Bug Fixes
  • Updated resolution logic to prioritize target object key order and adjust how the "default" condition is handled.
• Tests
  • Modified expected resolved paths and condition order in several test cases.
  • Removed the test for dual condition names.
• Chores
  • Removed an unused fixture and its workspace entry.

[coderabbitai]

Walkthrough

This change removes the dual-condition-names fixture, its workspace entry, and a related test. It also updates the logic in package_target_resolve to iterate over target object keys in insertion order instead of by condition priority, affecting how "default" is handled. Several test expectations and condition orders are updated accordingly.

Changes

Files/Paths	Change Summary
fixtures/dual-condition-names/package.json	Deleted the package fixture and its dependency declaration.
pnpm-workspace.yaml	Removed fixtures/dual-condition-names from the workspace package list.
src/lib.rs	Changed package_target_resolve to iterate over target object keys in insertion order, altering "default" handling.
src/tests/exports_field.rs, src/tests/imports_field.rs	Updated expected resolved paths and changed the order of condition_names in certain test cases.
tests/resolve_test.rs	Removed the dual_condition_names test function.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant Resolver

    Caller->>Resolver: package_target_resolve(target, conditions)
    alt target is object
        loop for each (key, value) in target (in insertion order)
            alt key == "default" or key in conditions
                Resolver->>Resolver: package_target_resolve(value, conditions)
                alt resolution found
                    Resolver-->>Caller: return resolution
                end
            end
        end
        Resolver-->>Caller: return None
    else
        Resolver-->>Caller: (handle other types)
    end

Loading

Possibly related PRs

• unrs/unrs-resolver#115: Reverses the logic for package_target_resolve and removes the fixture, workspace entry, and test for dual-condition-names, indicating a directly related but opposite change.

Poem

A fixture hops away, its test now gone,
The resolver's logic moves right along.
No more dual names in our bunny's den,
Conditions in order, checked once again.
With keys in a row, the code's now neat—
Another carrot found, a change complete! 🥕

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b929b5 and b56962d.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (6)

• fixtures/dual-condition-names/package.json (0 hunks)
• pnpm-workspace.yaml (0 hunks)
• src/lib.rs (1 hunks)
• src/tests/exports_field.rs (3 hunks)
• src/tests/imports_field.rs (2 hunks)
• tests/resolve_test.rs (0 hunks)

💤 Files with no reviewable changes (3)

• pnpm-workspace.yaml
• fixtures/dual-condition-names/package.json
• tests/resolve_test.rs

⏰ Context from checks skipped due to timeout of 90000ms (10)

• GitHub Check: Codacy Static Code Analysis
• GitHub Check: Test (windows-latest)
• GitHub Check: Lint
• GitHub Check: Test (ubuntu-latest)
• GitHub Check: Test wasi target
• GitHub Check: Test (macos-14)
• GitHub Check: Benchmark
• GitHub Check: Code Coverage
• GitHub Check: Analyze (actions)
• GitHub Check: Analyze (javascript-typescript)

🔇 Additional comments (6)

src/tests/imports_field.rs (2)

519-530:

❓ Verification inconclusive

Verify the behavioral change in condition resolution logic.

The expected result changed to select the "default" value ("./src/index.js") even when "browser" is specified in condition_names. This suggests the revert changed the resolution logic to prioritize JSON object insertion order over condition priority.

In the test case:

• The JSON object has "default" first, then "browser", then "node"
• condition_names includes "browser"
• But the test now expects the "default" value to be selected

This represents a significant behavioral change that could affect module resolution in real applications.

---

🏁 Script executed:

#!/bin/bash
# Description: Check for similar condition resolution changes across the codebase
# Expected: Find related changes in resolution logic and other test files

echo "=== Searching for condition resolution logic ==="
ast-grep --pattern 'condition_names'

echo "=== Searching for package target resolution ==="
rg -A 10 "package_target_resolve"

echo "=== Searching for similar test changes ==="
rg -A 5 -B 5 "Direct mapping #7"

Length of output: 23656

---

Confirm Intent of Condition Resolution Change

It looks like the revert has shifted resolution from honoring the specified condition_names priority to simply using the JSON object’s insertion order—choosing "default" first even when "browser" is requested. This is reflected in two updated tests:

• src/tests/imports_field.rs (Direct mapping feat: support resolving path with extra query #7): now expects "./src/index.js" despite condition_names: vec!["browser"]
• src/tests/exports_field.rs (Direct mapping feat: support resolving path with extra query #7): same change in exports tests

Please verify that choosing object property order over the explicit condition_names priority is intentional and compliant with the intended specification. You may want to review the package_target_resolve logic in src/lib.rs to ensure this behavior aligns with other resolution rules.

---

908-908:

✅ Verification successful

Confirm the condition order change aligns with the new resolution behavior.

The condition order was changed from vec!["webpack", "browser"] to vec!["browser", "webpack"]. Given that the test still expects the "webpack" result ("./wpk/index.mjs"), this suggests the resolution logic now prioritizes the JSON object insertion order where "webpack" appears first in the imports field.

This change is consistent with the revert's impact on condition resolution logic.

---

🏁 Script executed:

#!/bin/bash
# Description: Verify the JSON object key insertion order in this test case
# Expected: Confirm "webpack" appears before "browser" in the imports field

echo "=== Examining the test case structure ==="
rg -A 15 "conditional mapping folder #3" src/tests/imports_field.rs

Length of output: 679

---

Condition resolution respects mapping insertion order

The JSON object in this test is defined with keys in the order ["webpack", "browser", "node"]. Resolution now iterates over the imports‐field keys (in insertion order) and selects the first key that appears in the provided condition_names. Even though condition_names is now ["browser", "webpack"], "webpack" still wins because it comes first in the mapping.

• imports field key order: webpack → browser → node
• condition_names: ["browser", "webpack"]
• First matching key in the mapping is "webpack", yielding ./wpk/index.mjs

src/tests/exports_field.rs (3)

825-825: Update expected path for Direct mapping #7
The test now expects ./src/index.js instead of ./index.js, reflecting the reverted behavior where the "default" condition is resolved after "browser" and "node".

---

1347-1347: Swap condition_names order in conditional mapping folder #3
Changed from ["webpack", "browser"] to ["browser", "webpack"] to match the new insertion‐order iteration of conditions in package_target_resolve.

---

1365-1365: Swap condition_names order in conditional mapping folder #3 (wildcard)
Aligned wildcard test to use ["browser", "webpack"], consistent with the updated resolution priority.

src/lib.rs (1)

1807-1828: LGTM! Spec compliance improvement with behavior change.

This change correctly implements the Node.js ESM specification requirement to iterate over target object properties "in object insertion order" rather than by condition priority. The previous implementation that prioritized custom condition_names over insertion order was non-spec-compliant.

The new logic properly:

• Iterates over target.iter() in insertion order
• Handles both "default" and condition matching correctly
• Maintains efficient condition checking with conditions.iter().any()

Note that this revert represents a behavior change where the resolution order now depends on the object key insertion order rather than condition priority, which may affect existing code that relied on the previous non-standard behavior.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to b56962d in 1 minute and 22 seconds. Click for details.

• Reviewed 123 lines of code in 6 files
• Skipped 1 files when reviewing.
• Skipped posting 5 draft 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. tests/resolve_test.rs:1

• Draft comment:
  Overall, the test cases are comprehensive and cover many scenarios (including Unicode paths, symlink handling, different module formats, and long Windows paths).
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. tests/resolve_test.rs:125

• Draft comment:
  In the 'ipaddr_js' test, the second resolver uses extensions: vec![('.ts'.into())]. Consider using consistent syntax, e.g. vec![".ts".into()], for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. tests/resolve_test.rs:218

• Draft comment:
  The 'windows_symlinked_longfilename' test correctly asserts that the long Windows path is handled and includes a comment on canonicalization behavior, aiding cross-platform testing.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. tests/resolve_test.rs:240

• Draft comment:
  The 'package_json_with_bom' test verifies BOM removal from package JSON files. Consider adding a brief comment or renaming the test for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. tests/resolve_test.rs:20

• Draft comment:
  Tests for styled-components, axios and others are clearly separated with specific ResolveOptions. Usage of condition names and alias_fields is appropriate.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_rb6aTAjDG66tqHBQ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.40%. Comparing base (4b929b5) to head (b56962d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   93.41%   93.40%   -0.02%     
==========================================
  Files          13       13              
  Lines        2899     2894       -5     
==========================================
- Hits         2708     2703       -5     
  Misses        191      191              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

CodSpeed Performance Report

Merging #131 will not alter performance

_{Comparing fix/condition-names (b56962d) with main (4b929b5)}

Summary

✅ 3 untouched benchmarks