fix: check tsconfig matching before using resolver #473
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
🦋 Changeset detectedLatest commit: 2be84ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughMoved the resolver cache lookup in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant File as Source File
participant TSConfigs as tsconfig loop
participant Cache as Resolver Cache
participant Resolver as resolver creation
Note over File,TSConfigs: Old flow
File->>TSConfigs: iterate tsconfigs
TSConfigs->>Cache: check cache (early)
alt cached resolver found
Cache-->>Resolver: return cached resolver (break)
else
TSConfigs->>Resolver: match tsconfig -> create resolver
end
Note over File,TSConfigs: New flow (this PR)
File->>TSConfigs: iterate tsconfigs
TSConfigs->>TSConfigs: perform tsconfig match
alt tsconfig matched
TSConfigs->>Cache: check cache for matched tsconfig
alt cached resolver found
Cache-->>Resolver: return cached resolver (break)
else
TSConfigs->>Resolver: create and assign resolver
end
else
TSConfigs->>TSConfigs: continue loop
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
![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 6697e04 in 2 minutes and 42 seconds. Click for details.
- Reviewed
155lines of code in10files - Skipped
1files 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. src/index.ts:125
- Draft comment:
Reordered the resolver cache check after tsconfig matching to ensure the correct tsconfig is used. Consider adding an inline comment explaining this reordering for clarity. - 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 reordering does appear intentional and meaningful - we now only check the resolver cache after confirming the tsconfig matches the file. However, suggesting to add an explanatory comment is more of a nice-to-have than a critical issue. The code flow is already fairly clear from the structure and existing logging statements. The comment correctly identifies a meaningful code change, but requesting additional documentation is more of a preference than a clear issue. The existing code may already be sufficiently self-documenting. While documentation can be helpful, the code structure and existing log statements already make the flow clear. An additional inline comment isn't strictly necessary. This comment should be removed as it's suggesting an optional documentation improvement rather than pointing out a clear code issue that needs to be fixed.
2. tests/e2e/__snapshots__/e2e.spec.ts.snap:67
- Draft comment:
New snapshot for 'multipleTsconfigsWithReferences' has been added and appears correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/e2e/multipleTsconfigsWithReferences/.eslintrc.cjs:3
- Draft comment:
ESLint config correctly specifies the projects array with both tsconfig files. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. tests/e2e/multipleTsconfigsWithReferences/backend/index.ts:1
- Draft comment:
Backend test file demonstrates proper use of both relative imports and tsconfig path mappings. Looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. tests/e2e/multipleTsconfigsWithReferences/backend/utils.ts:1
- Draft comment:
Backend utility file is simple and correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. tests/e2e/multipleTsconfigsWithReferences/frontend/component.tsx:1
- Draft comment:
Frontend component file is minimal and defined correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. tests/e2e/multipleTsconfigsWithReferences/frontend/index.tsx:1
- Draft comment:
Frontend index file properly tests both relative imports and tsconfig path mappings; no issues observed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. tests/e2e/multipleTsconfigsWithReferences/shared-utils/helper.ts:1
- Draft comment:
Shared utility helper is defined correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. tests/e2e/multipleTsconfigsWithReferences/tsconfig.node.json:5
- Draft comment:
tsconfig.node.json is properly configured with the correct 'include' patterns and path mappings for backend and shared utils. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. tests/e2e/multipleTsconfigsWithReferences/tsconfig.web.json:5
- Draft comment:
tsconfig.web.json is correctly set up for frontend with JSX and shared utils path mappings. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. tests/e2e/multipleTsconfigsWithReferences/backend/index.ts:8
- Draft comment:
Typographical suggestion: Consider changing "node_module" to "node_modules" in this comment for consistency with common terminology. - 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 suggestion is technically correct (the plural form 'node_modules' is more common), this is an extremely minor documentation issue. The comment is clear enough as is and doesn't impact code functionality. The rules specifically say not to make purely informative comments and to avoid obvious or unimportant changes. The typo could theoretically confuse new developers about the correct terminology. Also, since this is a new file, it's better to start with correct terminology. The meaning is still perfectly clear despite the minor typo, and fixing it provides no real value. This is exactly the kind of nitpicky comment that clutters PR reviews. Delete this comment as it's purely cosmetic, doesn't affect functionality, and violates the rule about not making purely informative comments.
12. tests/e2e/multipleTsconfigsWithReferences/frontend/index.tsx:8
- Draft comment:
Typographical note: The comment says "import from node_module". Consider using "node_modules" to reflect the standard naming convention. - 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 technically correct that "node_modules" is the standard term, this is just a comment typo that doesn't affect functionality. The comment is purely informative and doesn't require any actual code changes. According to the rules, we should not make purely informative comments or comments about unimportant issues. The typo could potentially confuse future developers about the correct terminology. Comments are part of code documentation. The difference is minor and obvious enough that it wouldn't cause real confusion. The meaning is clear even with the typo. Delete the comment as it's purely informative and about an unimportant typo that doesn't affect functionality.
Workflow ID: wflow_hzWW56KfTjK9lpxB
You can customize 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR fixes an issue where the resolver was using a cached resolver before determining which tsconfig file actually includes the current file when multiple tsconfig files are specified for the same directory path.
Problem:
When multiple tsconfig files (e.g.,
tsconfig.node.jsonandtsconfig.web.json) exist in the same directory with differentincludepatterns, the cached resolver was used before matching which tsconfig includes the current file. This caused incorrect tsconfig settings to be applied.Solution:
Reorder the resolver cache check to occur after tsconfig matching. This ensures the correct tsconfig configuration is identified before attempting to use a cached resolver.
Changes:
src/index.tsto move resolver cache check from lines 101-107 to after tsconfig matching (now at lines 133-139)multipleTsconfigsWithReferences:backend/**/*) correctly usetsconfig.node.jsonwith@backend/*pathsfrontend/**/*) correctly usetsconfig.web.jsonwith@frontend/*paths@shared/*pathsImportant
Fix resolver cache check order in
src/index.tsto ensure correct tsconfig is identified before using cached resolver, with new e2e test added.resolve()insrc/index.tsto occur after tsconfig matching, ensuring correct tsconfig is identified before using cached resolver.multipleTsconfigsWithReferencesto verify correct tsconfig usage for backend and frontend files.tsconfig.node.jsonand frontend files usetsconfig.web.json, both resolving shared utilities via@shared/*paths.This description was created by
for 6697e04. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests
Chores