fix: support resolving abnormal relative paths with node_modules
#171
Conversation
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
WalkthroughThe changes introduce handling for abnormal relative module specifiers (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Resolver
participant PathUtil
Caller->>Resolver: load_package_self_or_node_modules(specifier)
alt Specifier contains abnormal relative segments
Resolver->>PathUtil: normalize_relative(specifier)
PathUtil-->>Resolver: normalized_specifier
Resolver->>Resolver: parse_package_name(normalized_specifier)
alt package_name == ".."
Resolver->>Resolver: load_node_modules(normalized_specifier)
alt Success
Resolver-->>Caller: Resolved path
else Failure
Resolver-->>Caller: NotFound error
end
else
Resolver-->>Caller: NotFound error
end
else
Resolver->>Resolver: (existing resolution logic)
Resolver-->>Caller: (existing result)
end
Assessment against linked issues
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 (
|
![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 cce1f34 in 44 seconds. Click for details.
- Reviewed
49lines of code in5files - 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/abnormal-relative/foo/bar/baz/test.js:1
- Draft comment:
Remove unused import of 'node:path' if not needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. fixtures/abnormal-relative/foo/bar/baz/test.js:3
- Draft comment:
Clarify the purpose of using the abnormal relative request 'jest-runner-../../../'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. fixtures/abnormal-relative/runner.js:1
- Draft comment:
If the empty runner.js is intentional, add a comment indicating its role. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/tests/resolve.rs:197
- Draft comment:
Add inline documentation in the 'abnormal_relative' test to explain why resolving 'jest-runner-../../../' should yield runner.js. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_lUWYqPv6YsisO8hZ
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: 0
🧹 Nitpick comments (1)
fixtures/abnormal-relative/foo/bar/baz/test.js (1)
1-1: Remove unused import.The
pathmodule is imported but never used in this test file.-const path = require('node:path')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fixtures/abnormal-relative/node_modules/.gitkeepis excluded by!**/node_modules/**fixtures/abnormal-relative/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
fixtures/abnormal-relative/foo/bar/baz/test.js(1 hunks)fixtures/abnormal-relative/package.json(1 hunks)src/tests/resolve.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:17:34.175Z
Learning: Test cases in unrs-resolver that intentionally document failing behavior should make the intent clear by either: 1) asserting on the expected error condition rather than the desired behavior, 2) using #[should_panic] for tests expected to panic, or 3) using #[ignore] with a reason to skip known failing tests in CI.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:13:59.358Z
Learning: In the unrs-resolver project, tests that intentionally document failing behavior (regression tests) should either use #[should_panic], assert on the expected error, or include clear documentation indicating they are intentional failing test cases.
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#115
File: tests/resolve_test.rs:251-263
Timestamp: 2025-05-28T02:21:41.547Z
Learning: In pnpm workspaces, `pnpm install` must be run first to initialize dependencies and create the proper node_modules/.pnpm structure before tests can pass. Tests that rely on resolving packages will fail if dependencies haven't been installed yet.
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
fixtures/abnormal-relative/foo/bar/baz/test.js (8)
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
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#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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`.
fixtures/abnormal-relative/package.json (7)
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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#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#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
src/tests/resolve.rs (9)
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:13:59.358Z
Learning: In the unrs-resolver project, tests that intentionally document failing behavior (regression tests) should either use #[should_panic], assert on the expected error, or include clear documentation indicating they are intentional failing test cases.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:17:34.175Z
Learning: Test cases in unrs-resolver that intentionally document failing behavior should make the intent clear by either: 1) asserting on the expected error condition rather than the desired behavior, 2) using #[should_panic] for tests expected to panic, or 3) using #[ignore] with a reason to skip known failing tests in CI.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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#129
File: src/tests/pnp.rs:6-6
Timestamp: 2025-06-02T15:57:50.888Z
Learning: In the unrs-resolver project, std::env is acceptable for test cases even when deprecated, as the user prefers simpler approaches in test code compared to production code standards.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Test (macos-14)
- GitHub Check: Test wasi target
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint
- GitHub Check: Code Coverage
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (3)
fixtures/abnormal-relative/package.json (1)
1-6: LGTM! Test fixture package.json is well-structured.The minimal package.json configuration is appropriate for the test fixture, with the main field correctly pointing to "runner.js" as expected by the resolver test.
fixtures/abnormal-relative/foo/bar/baz/test.js (1)
3-3: LGTM! Core test logic is correct.The require.resolve call effectively tests the runtime resolution behavior for the abnormal relative path, complementing the resolver unit test.
src/tests/resolve.rs (1)
196-205: Clarify test intent: failing vs. passing test case.The PR title indicates this is a "failing test case," but the test asserts on successful resolution without any failing test markers. Based on the project's patterns for documenting failing behavior, consider:
- If this test should fail: Use
#[should_panic],#[ignore], or assert on the expected error condition- If this test should pass: The current implementation is correct
The core test logic for abnormal relative path resolution looks sound.
Could you clarify whether this test is expected to pass or fail? If it's meant to document failing behavior, it should follow the established patterns for failing tests in this codebase.
JounQin
changed the title
JounQin
force-pushed
the
fix/abnormal_relative
branch
from
72beea0 to
8b40466
Compare
|
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 8b40466 in 37 seconds. Click for details.
- Reviewed
58lines of code in4files - 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. fixtures/abnormal-relative/foo/bar/baz/test.js:1
- Draft comment:
Consider using an assertion instead of console.log to automatically verify the resolution outcome. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. fixtures/abnormal-relative/package.json:4
- Draft comment:
The 'main' file 'runner.js' is referenced but not shown in the diff; please confirm that it exists as expected. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. fixtures/pnpm-workspace/package.json:10
- Draft comment:
The dependency 'enhanced-resolve' has been bumped to ^5.18.2; ensure that corresponding lockfile updates are applied if needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/tests/resolve.rs:196
- Draft comment:
The new 'abnormal_relative' test effectively validates abnormal relative path resolution. Consider adding more edge cases if similar patterns exist. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_zjCVewTk3Z9fQngM
You can customize
JounQin
changed the title
|
JounQin
changed the title
node_modules
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
+ Coverage 92.22% 92.30% +0.08%
==========================================
Files 13 13
Lines 3060 3106 +46
==========================================
+ Hits 2822 2867 +45
- Misses 238 239 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib.rs (1)
597-626: Well-implemented legacy compatibility for abnormal relative specifiers.The implementation correctly handles the edge case described in Jest issue #15712:
Strengths:
- Properly detects abnormal relative patterns (
"/../.."and"../../")- Uses the new
normalize_relativemethod appropriately- Preserves trailing slash semantics
- Only attempts legacy resolution when
package_name == ".."- Includes clear documentation and references
Design considerations:
- The TODO comment suggests adding a configuration option for this legacy behavior, which would be beneficial for users who want to opt out
- The implementation correctly limits this to
requirecontexts (not ESM) as indicated by the commentsThe logic flow is sound and maintains backward compatibility while being clearly scoped to specific abnormal cases.
Consider adding the configuration option mentioned in the TODO comment:
// TODO: add a new option for this legacy behavior? + if !self.options.enable_legacy_abnormal_relative { + return Err(ResolveError::NotFound(specifier.to_string())); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fixtures/abnormal-relative-with-node_modules/node_modules/.gitkeepis excluded by!**/node_modules/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.js(1 hunks)fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.mjs(1 hunks)fixtures/abnormal-relative-with-node_modules/package.json(1 hunks)fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.js(1 hunks)fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.mjs(1 hunks)fixtures/abnormal-relative-without-node_modules/package.json(1 hunks)fixtures/pnpm-workspace/package.json(1 hunks)src/lib.rs(1 hunks)src/path.rs(3 hunks)src/tests/resolve.rs(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- fixtures/abnormal-relative-with-node_modules/package.json
- fixtures/pnpm-workspace/package.json
- fixtures/abnormal-relative-without-node_modules/package.json
- fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.js
- fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/resolve.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
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.
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#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.mjs (8)
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
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#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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`.
fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.mjs (8)
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#144
File: napi/patch.mjs:5-5
Timestamp: 2025-06-10T13:53:02.983Z
Learning: Build-time scripts in napi/patch.mjs and similar build pipeline contexts don't require error handling for reading expected files like index.js, as these files are guaranteed to be present during the build process.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
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#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
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`.
src/lib.rs (6)
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T09:52:45.840Z
Learning: In Yarn PnP environments, some packages use nested package.json files for subpaths instead of the exports field. The resolver should check for these nested package.json files when resolving subpaths like `@atlaskit/pragmatic-drag-and-drop/combine` to correctly handle packages with this structure.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:0-0
Timestamp: 2025-04-21T06:11:46.964Z
Learning: JounQin's fix for the dot alias issue in PR #72 addresses the problem by correctly normalizing paths in the `extend_tsconfig` method, ensuring that base URLs are properly resolved as absolute paths when extending tsconfigs, rather than adding special case handling for dot aliases in the resolver.
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.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: src/tests/pnp.rs:116-133
Timestamp: 2025-04-21T08:17:34.175Z
Learning: Test cases in unrs-resolver that intentionally document failing behavior should make the intent clear by either: 1) asserting on the expected error condition rather than the desired behavior, 2) using #[should_panic] for tests expected to panic, or 3) using #[ignore] with a reason to skip known failing tests in CI.
Learnt from: JounQin
PR: unrs/unrs-resolver#72
File: fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4-6
Timestamp: 2025-04-20T16:00:16.106Z
Learning: The configuration with `".": ["."]` in the paths setting in fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json is an intentional test case for testing the TypeScript dot alias resolution behavior, specifically related to issue #437 in the eslint-import-resolver-typescript repository.
🧬 Code Graph Analysis (1)
src/path.rs (1)
src/tests/missing.rs (1)
test(8-70)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Test (macos-14)
- GitHub Check: Test (windows-latest)
- GitHub Check: Benchmark
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
src/path.rs (3)
19-20: LGTM! Clear documentation for the new method.The method signature and documentation clearly distinguish this from the existing
normalizemethod by explicitly stating it doesn't require absolute paths.
65-79: LGTM! Correct implementation of relative path normalization.The implementation correctly handles the different path components:
ParentDir: Attempts to pop, or adds..if no components to popCurDir: Properly ignored for normalization- Other components: Preserved as-is
This is the appropriate behavior for relative path normalization without absolute path requirements.
153-159: Comprehensive test coverage for the new functionality.The test cases effectively cover:
- Multiple parent directory traversals (
foo/../../foo/→../foo)- Mixed current/parent directories (
foo/.././foo/→foo)- Edge cases like
jest-runner-../../→ empty string- Various path component combinations
The test cases align well with the intended use case for abnormal relative specifiers.
fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.mjs (1)
1-1: Appropriate test fixture for ESM abnormal relative resolution.This fixture correctly tests the ESM behavior where
import.meta.resolvewith abnormal relative specifiers should fail, consistent with the implementation that only provides legacy support forrequirecontexts.fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.mjs (1)
1-1: Consistent test fixture for environments without node_modules.This fixture appropriately tests the same ESM abnormal relative resolution behavior in an environment without
node_modules, ensuring consistent error handling across different package layouts.
CodSpeed Performance ReportMerging #171 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5144e04 in 2 minutes and 22 seconds. Click for details.
- Reviewed
237lines of code in11files - Skipped
1files when reviewing. - Skipped posting
5draft 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:600
- Draft comment:
The new branch for handling abnormal relative specifiers uses a broad check (contains '/../..' || '../../'). Consider tightening this matching (e.g. via a regex) to avoid unintended matches, and ensure it only applies to CommonJS require calls, not to ESM imports. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The current string-based check is actually quite clear and matches the documented intent - it's looking for parent directory traversal patterns. The regex suggestion would make the code more complex without clear benefits. The comment also misunderstands that this is intentionally handling a legacy edge case, not a bug that needs to be fixed. The current implementation is sufficient for the documented use case. I could be wrong about the regex being more complex - a well-crafted regex might actually be more precise and catch edge cases I haven't considered. The comment does raise a valid point about being more precise with the matching. While regexes can be more precise, in this case the simple string contains() check is adequate since we're just looking for specific substrings. The code comment explicitly states this is handling a legacy edge case, so being overly precise isn't necessary. The comment should be deleted. The current string-based check is clear and sufficient for the documented legacy behavior it's handling. Making it more complex with regex would not provide meaningful benefits.
2. src/path.rs:65
- Draft comment:
The implementation of normalize_relative is clear. A minor suggestion: add a brief inline comment explaining that leading ParentDir components are preserved when no preceding segment exists, to aid future maintainers. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. src/tests/resolve.rs:197
- Draft comment:
Tests for abnormal relative paths are thorough. It might be beneficial to add explicit tests verifying that abnormal relative specifiers do not resolve in ESM contexts (e.g. via import.meta.resolve) as indicated by the comments in the fix. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.js:3
- Draft comment:
It appears that the string'jest-runner-/../..'in the second console.log might contain an extra dash afterjest-runner. Please confirm if this is intentional or if it should be corrected to match the other patterns (perhaps'jest-runner-../..'or another intended format). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This appears to be a test file specifically testing different path patterns with require.resolve. The varying patterns, including the one with the extra dash, seem intentional as they're testing different cases. The comment at line 5 marking one as "error" suggests these are deliberate test cases. The dash is likely part of the test coverage. I could be wrong about the intention - maybe this is not a test file and these are actual module imports that need to work correctly. The file is in a "fixtures" directory and has a systematic variation of patterns, strongly suggesting these are intentional test cases rather than actual imports. The comment should be deleted as it's questioning what appears to be an intentional test case pattern, and we shouldn't question the author's intention in test fixtures.
5. src/path.rs:157
- Draft comment:
Typo alert: The string "foo../../.." may be missing a slash after "foo". Did you intend to use "foo/../../.." for consistency with the other test cases? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The normalize_relative() function works by iterating through path components. The Rust Path implementation can handle paths with inconsistent slashes. The test case is valid and produces the expected "../" result, showing that the function correctly handles parent directory references even with unusual formatting. Adding a slash wouldn't change the behavior or improve test coverage. The comment author might be right that consistency in test case formatting could make the code more readable. The unusual formatting might confuse future readers. While consistent formatting is nice, the current test case is valid and actually provides value by testing edge case path parsing. The unusual format helps ensure the function works with imperfect input. Delete the comment. The test case is valid as-is and tests important edge case behavior. The suggested change wouldn't improve functionality.
Workflow ID: wflow_nN6dDdhQVdG1VvZi
You can customize
There was a problem hiding this comment.
Pull Request Overview
Adds support for resolving abnormal relative paths through a new normalize_relative helper and updated resolver logic, with accompanying tests and fixture updates.
- Introduces
normalize_relative()inpath.rsand adds tests for it - Updates resolver in
lib.rsto handle legacy abnormal relative specifiers - Adds
abnormal_relativetests inresolve.rsand bumpsenhanced-resolvedependency
Reviewed Changes
Copilot reviewed 4 out of 14 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/path.rs | Added normalize_relative method and its unit tests |
| src/lib.rs | Enhanced resolver logic to support abnormal relative paths |
| src/tests/resolve.rs | New abnormal_relative test covering both fixture cases |
| fixtures/pnpm-workspace/package.json | Bumped enhanced-resolve from ^5.18.1 to ^5.18.2 |
Files not reviewed (7)
- fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.js: Language not supported
- fixtures/abnormal-relative-with-node_modules/foo/bar/baz/test.mjs: Language not supported
- fixtures/abnormal-relative-with-node_modules/package.json: Language not supported
- fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.js: Language not supported
- fixtures/abnormal-relative-without-node_modules/foo/bar/baz/test.mjs: Language not supported
- fixtures/abnormal-relative-without-node_modules/package.json: Language not supported
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
src/lib.rs:604
- [nitpick] The variable name
ownedis generic. Consider renaming it to something likenormalized_stringornormalized_specifierfor clarity.
let mut owned = path.to_string_lossy().into_owned();
src/path.rs:157
- The test input lacks a slash before the parent‐directory segments, so
components()won’t split them correctly. Change toPath::new("foo/../../..").normalize_relative()to match the expected behavior.
assert_eq!(Path::new("foo../../..").normalize_relative(), Path::new(".."));
src/path.rs:158
- The path literal should include a slash before the
..so it’s treated as a separate ParentDir component. For example usePath::new("jest-runner-/../../").normalize_relative().
assert_eq!(Path::new("jest-runner-../../").normalize_relative(), Path::new(""));
JounQin
commented
Jul 2, 2025
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.
close jestjs/jest#15712
Important
Adds support for resolving abnormal relative paths in module requests by introducing
normalize_relative()and updating resolver logic.jest-runner-../../..inlib.rs.normalize_relative()inpath.rsto handle non-absolute path normalization.abnormal_relative()test inresolve.rsto verify handling of abnormal relative paths.abnormal-relative-with-node_modulesandabnormal-relative-without-node_modulesadded.enhanced-resolveversion inpnpm-workspace/package.json.This description was created by
for 5144e04. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit