fix: resolve parent base url correctly by normalizing as absolute path #72
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
WalkthroughThis update introduces a new test scenario for TypeScript configuration path resolution, specifically addressing the "parent-base-url" case. Several new fixture files are added, including TypeScript source files and multiple tsconfig variants to simulate inheritance and base URL resolution. The Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Resolver
participant TsConfig
participant FileSystem
TestRunner->>Resolver: Resolve module "index" from test/
Resolver->>TsConfig: Load tsconfig.json (extends tsconfig.dev)
TsConfig->>TsConfig: Extend from tsconfig.dev.json
TsConfig->>TsConfig: Extend from tsconfig.base.json (baseUrl: ../src)
TsConfig->>FileSystem: Normalize and resolve baseUrl
Resolver->>FileSystem: Locate src/index.ts
FileSystem-->>Resolver: Return resolved path
Resolver-->>TestRunner: Return resolution result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (7)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
✨ 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:
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 adds a failing test case to verify the dot alias functionality as raised in issue #437.
- Introduces a new test function in tsconfig_paths.rs to cover dot alias resolution.
- Adds new fixture files: a test file that re-exports an alias and a source file that exports a constant for the alias.
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tests/tsconfig_paths.rs | Added new test function to verify dot alias resolution. |
| fixtures/tsconfig/cases/dot-alias/test/story.ts | Added test file that re-exports from the alias index. |
| fixtures/tsconfig/cases/dot-alias/src/index.ts | Added source file exporting a constant used in the dot alias test. |
Files not reviewed (3)
- fixtures/tsconfig/cases/dot-alias/tsconfig.json: Language not supported
- fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json: Language not supported
- fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.dev.json: Language not supported
Comments suppressed due to low confidence (2)
src/tests/tsconfig_paths.rs:302
- [nitpick] Consider renaming 'f' to a more descriptive name like 'tsconfig_root' to clarify its purpose.
let f = super::fixture_root().join("tsconfig");
src/tests/tsconfig_paths.rs:303
- [nitpick] Consider renaming 'f2' to 'dot_alias_path' or a similarly descriptive name to improve code clarity.
let f2 = f.join("cases").join("dot-alias");
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to f26d03b in 2 minutes and 24 seconds. Click for details.
- Reviewed
71lines of code in5files - Skipped
1files when reviewing. - Skipped posting
6draft 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/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4
- Draft comment:
Using "." as an alias key is unusual. Please double-check that the intent is to map "." to ["."] as the failing case. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the author to double-check their intention, which violates the rules. However, it also points out an unusual use of "." as an alias key, which could be a valid concern. The comment could be rephrased to suggest a specific alternative or ask for confirmation of the intention without asking for a double-check.
2. src/tests/tsconfig_paths.rs:310
- Draft comment:
The dot alias test references a tsconfig file named "tsconfig.json" (via dir.parent().join(tsconfig)), but only tsconfig.base.json and tsconfig.dev.json are provided in this case. Confirm whether a tsconfig.json file should be added or if the test should reference tsconfig.dev.json instead. - 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% 1. The comment makes an assumption about what files exist in the test fixtures, but we don't have access to the actual test fixtures. 2. The test is clearly written to test a specific case with tsconfig.json. 3. Without seeing the test fixtures, we can't verify if there's actually a problem. 4. The test passes (otherwise it would have failed in CI), suggesting tsconfig.json does exist. I could be wrong in assuming the test passes just because it's in the codebase - maybe it's a new test that hasn't run yet. Also, the commenter may have access to the test fixtures that I don't. Even with those possibilities, suggesting a change to a test without clear evidence of a problem is risky. If there was a real issue, the test would fail and provide clear evidence something needs to be fixed. The comment should be deleted as it makes assumptions about test fixtures we can't verify, and suggests changing a test that appears to be working as intended.
3. fixtures/tsconfig/cases/dot-alias/src/index.ts:1
- Draft comment:
Minimal export file looks fine. Consider ensuring a trailing newline per style guidelines. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. fixtures/tsconfig/cases/dot-alias/test/story.ts:1
- Draft comment:
Non‐relative import ('index') is used intentionally to trigger alias resolution; consider adding a brief comment to clarify its purpose. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:4
- Draft comment:
The use of the dot alias ('.') in the paths mapping is unconventional. Please add a comment explaining its intended use to aid future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/tests/tsconfig_paths.rs:299
- Draft comment:
In test_paths_dot_alias, the test constructs the tsconfig path as 'tsconfig.json' from the parent folder, but no such file exists in the dot-alias case (only tsconfig.base.json and tsconfig.dev.json are provided). Confirm if this setup is intentional for reproducing issue #437 or if it should reference the proper tsconfig file. - 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% 1. The test is currently passing with "tsconfig.json", which suggests it's working as intended 2. Without access to the actual test fixtures, I can't verify the commenter's claim about which files exist 3. Even if tsconfig.json doesn't exist, the test may be intentionally testing behavior with a missing config file 4. The comment is speculative, asking for confirmation rather than stating a clear issue I don't have access to the actual test fixtures to verify which config files exist. The comment could be correct about the files. However, the test is passing as written, and the comment is speculative rather than identifying a clear issue. The test may be intentionally testing behavior with a non-existent config file. Delete the comment. It's speculative, asks for confirmation rather than identifying a clear issue, and the test is currently passing as written.
Workflow ID: wflow_D94UM6aP26cJn9wY
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 selected for processing (6)
fixtures/tsconfig/cases/dot-alias/src/index.ts(1 hunks)fixtures/tsconfig/cases/dot-alias/test/story.ts(1 hunks)fixtures/tsconfig/cases/dot-alias/tsconfig.json(1 hunks)fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json(1 hunks)fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.dev.json(1 hunks)src/tests/tsconfig_paths.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code Coverage
src/tests/tsconfig_paths.rs
[error] 319-319: Test failure in 'tests::tsconfig_paths::test_paths_dot_alias': assertion left == right failed comparing index tsconfig.json path. Expected Ok with path '/home/runner/work/unrs-resolver/unrs-resolver/fixtures/tsconfig/cases/dot-alias/src/index.ts' but got Err(NotFound("index")).
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (4)
fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.dev.json (1)
2-3: Extends reference is valid
Using an extensionless"extends": "./tsconfig.base"is supported by TypeScript and correctly inherits the base settings.fixtures/tsconfig/cases/dot-alias/src/index.ts (1)
1-1: Simple export looks good
The module correctly exportsfooas a constant. No issues detected.fixtures/tsconfig/cases/dot-alias/tsconfig.json (1)
2-3: tsconfig extends chain is correct
Extending./tsconfigs/tsconfig.devproperly inherits baseUrl and paths from the layered configs.fixtures/tsconfig/cases/dot-alias/test/story.ts (1)
1-1: Re-export style is appropriate
Theexport * from "index";statement cleanly tests the dot‑alias resolution.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 93.20% 93.22% +0.02%
==========================================
Files 13 13
Lines 2841 2850 +9
==========================================
+ Hits 2648 2657 +9
Misses 193 193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7caf9fd in 1 minute and 33 seconds. Click for details.
- Reviewed
103lines of code in5files - Skipped
0files when reviewing. - Skipped posting
12draft 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. .editorconfig:12
- Draft comment:
Adding custom indent_size for Rust files is fine. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. fixtures/tsconfig/cases/parent-base-url/tsconfigs/tsconfig.base.json:1
- Draft comment:
Parent-base-url tsconfig is added correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/tests/tsconfig_paths.rs:300
- Draft comment:
Renaming test from 'test_paths_dot_alias' to 'test_parent_base_url' clarifies the purpose. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/tests/tsconfig_paths.rs:307
- Draft comment:
Ensure comparing Err(ResolveError::NotFound(".".into())) matches the intended error from resolver. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/tsconfig.rs:88
- Draft comment:
Normalization logic in extend_tsconfig is duplicated; consider refactoring for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/tsconfig.rs:196
- Draft comment:
Substituting the template variable appears correct; verify that 'trim_start_matches' handles extra slashes consistently. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. .editorconfig:12
- Draft comment:
Added rust-specific indent_size; this looks consistent with your coding style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. fixtures/tsconfig/cases/dot-alias/tsconfigs/tsconfig.base.json:1
- Draft comment:
Removed the dot-alias fixture. Ensure that any dot alias behavior is now covered by the 'parent-base-url' tests. - 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.
9. src/tests/tsconfig_paths.rs:301
- Draft comment:
Renamed test from 'test_paths_dot_alias' to 'test_parent_base_url' and added cases for both error ('.') and valid resolution ('index'); this improves clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing what was done in the code without providing any actionable feedback or suggestions. It does not ask for confirmation or suggest improvements, nor does it point out any potential issues. Therefore, it violates the rule against making purely informative comments.
10. src/tsconfig.rs:87
- Draft comment:
In extend_tsconfig, baseUrl is now conditionally normalized based on the TEMPLATE_VARIABLE. This ensures that paths with '${configDir}' remain unresolved for later substitution. Looks good. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what the code does without suggesting any changes or asking for clarification. It doesn't provide any actionable feedback or raise any concerns about the code.
11. src/tsconfig.rs:96
- Draft comment:
The normalization logic for setting paths_base now similarly checks for TEMPLATE_VARIABLE, ensuring consistency with baseUrl handling. This is a solid improvement. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what the code does without suggesting any changes or asking for clarification. It doesn't align with the rules for useful comments.
12. .editorconfig:13
- Draft comment:
Typographical note: The configuration line 'indent_size = 4' on line 13 has spaces around the '=' sign, which is inconsistent with the rest of the file (e.g., 'indent_size=2' on line 5). For consistency, consider removing the extra spaces to use 'indent_size=4'. - 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% While the comment is technically correct about the inconsistency, it's an extremely minor style issue in a config file. The functionality is identical either way. The comment doesn't point out any actual problems with the code or configuration logic. We should focus on more substantial issues. The inconsistency could make the file slightly harder to maintain if different developers follow different styles. Config files benefit from strict consistency. While consistency is good, this is too minor to warrant a comment. The functionality is identical and the inconsistency is trivial. Delete this comment as it focuses on an extremely minor style issue that doesn't impact functionality.
Workflow ID: wflow_IRP4r5l0Xh6fezbR
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
CodSpeed Performance ReportMerging #72 will not alter performanceComparing Summary
|
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 import-js/eslint-import-resolver-typescript#437
Important
Add support for dot-alias path mapping in TypeScript configurations and a test case to verify it.
tsconfig.rs.test_parent_base_url()intsconfig_paths.rsto verify module resolution using dot-alias path mapping..editorconfigto setindent_sizefor Rust files.This description was created by
for 7caf9fd. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit