fix(auto-import): don't suggest # imports that only resolve via condition fallback - #64177
Open
Marwan (marwan562) wants to merge 2 commits into
Open
fix(auto-import): don't suggest # imports that only resolve via condition fallback#64177Marwan (marwan562) wants to merge 2 commits into
Marwan (marwan562) wants to merge 2 commits into
Conversation
…tion fallback Reverse mapping in tryGetModuleNameFromExportsOrImports mirrored TS resolver fallback across conditions, suggesting specifiers like #utils/summarize/summarize that resolve via default only after node misses. Node picks first matching condition and throws on miss, so such suggestions crash at runtime with ERR_MODULE_NOT_FOUND. Mirror Node first-match semantics: when a runtime-active condition misses, block later conditions. Types-only conditions are ignored at runtime and don't block. Handles nested conditionals without active runtime keys and preserves array fallback. Closes microsoft#64171
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate resolution-semantics issues could still produce incorrect auto-import behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates auto-import reverse mapping to better match Node.js conditional import resolution.
Changes:
- Stops fallback after matching runtime conditions.
- Adds conditional, nested, types-only, and array mapping tests.
File summaries
| File | Review |
|---|---|
tsc/internal/modulespecifiers/specifiers.go |
Requires fixes for deeper conditional objects and Node array-target semantics. |
tsc/internal/modulespecifiers/specifiers_test.go |
Adds regression coverage, but incorrectly expects file-existence fallback for arrays. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Author
|
@microsoft-github-policy-service agree |
…rays Address Copilot review: arrays select first valid string target at runtime (no file-existence fallback), and deeper nested conditionals with inactive keys return undefined and should fallback. Switch reverse mapping to tri-state (matched/blocked/skipped) so terminal misses block later fallback while undefined continues.
Author
|
Addressed both Copilot comments:
Added deeper-nesting + array-undefined-first cases. All Copilot review |
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
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.
With
imports": { "#*": { "node": "./dist/*/index.js", "default": "./dist/*.js" } }and nodenext, auto-import offered both#utils/summarizeand#utils/summarize/summarize. The second only resolves in TS by falling back fromnodetodefaultafter thenodetarget misses. Node picks the first matching condition and throwsERR_MODULE_NOT_FOUNDinstead, so picking that suggestion crashes at runtime.Reverse mapping in
tryGetModuleNameFromExportsOrImportsdid the same fallback as the resolver. It now mirrors Node first-match: when a runtime-active condition misses, later conditions aren't tried.types/types@don't block since Node ignores them, nested objects with no active runtime key still fall through, and array fallback still works.Added cases alongside the existing ones in
tsc/internal/modulespecifiers/specifiers_test.go.Fixes #64171
Related #62439, #50762
Disclosure: I used AI assistance to draft this patch and reviewed and tested it myself.