fix(resolution): resolve .js import specifiers to their TS source - #1484
Open
valkyriweb wants to merge 1 commit into
Open
fix(resolution): resolve .js import specifiers to their TS source#1484valkyriweb wants to merge 1 commit into
.js import specifiers to their TS source#1484valkyriweb wants to merge 1 commit into
Conversation
`EXTENSION_RESOLUTION` only ever appends a suffix, so a NodeNext-style specifier `./x.js` was tried as `x.js.ts`, `x.js.tsx`, ... and finally bare `x.js` — never `x.ts`. `resolveRelativeImport` therefore returned null. Because `"moduleResolution": "NodeNext"` requires the emitted extension on relative imports, an entire class of TypeScript projects resolved no relative import at all: every reference fell through to the name-matcher, which cannot cross a rename, so aliased and default imports reported a false 0 callers. One 16k-file project measured 32,732 `.js` specifiers and 0 extensionless. Adds a rewrite table (`.js` -> `.ts`/`.tsx`/`.d.ts`, `.mjs` -> `.mts`, `.cjs` -> `.cts`) applied in `resolveRelativeImport` and `resolveAliasedImport`, tried only AFTER the literal path so a real emitted `.js` on disk still wins and no already-resolving import changes target. Each fixture imports under a different local name than the declaration, so the name-matcher cannot rescue the edge and the test isolates import resolution. Refs: colbymchenry#1482
Author
|
Companion: #1485 (alias bindings). Independent — either order; whichever lands second may need a trivial rebase of |
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
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.
Fixes the first of the two root causes behind #1482.
The bug
EXTENSION_RESOLUTIONonly ever appends a suffix. So a NodeNext-style relative specifier is tried as:x.tsis never tried.resolveRelativeImportreturnsnull.That matters more than it looks, because
"moduleResolution": "NodeNext"requires the emitted extension on relative imports. In such a project no relative import resolves at all — every reference falls through to Strategy 3, the name-matcher. Name-matching cannot cross a rename, so a default import or an aliased import silently reports 0 callers with full confidence.One 16k-file TypeScript project I measured has 32,732
.jsspecifiers and 0 extensionless ones. For that repo, import-based resolution was effectively off.The fix
A small rewrite table, tried only after the literal path:
.js.ts,.tsx,.d.ts.jsx.tsx,.jsx,.d.ts.mjs.mts,.d.mts.cjs.cts,.d.ctsApplied in
resolveRelativeImportand inresolveAliasedImport'stryWithExt, gated to TS-family languages.Ordering is the safety property: a real emitted
.jssitting on disk beside its source still wins, so no currently-resolving import changes target. There is a test for exactly that.Tests
__tests__/js-specifier-ts-resolution.test.ts, 3 cases:./impl.js— resolves toimpl.ts.mjs/.cjs->.mts/.cts./emitted.jswhere bothemitted.jsandemitted.tsexist — the real.jswinsEach fixture imports under a different local name than the declaration, so the name-matcher cannot rescue the edge; the assertion isolates import resolution.
On
maincases 1 and 2 fail and case 3 passes; with the fix all three pass.Verification
kernel-*-parityfiles, expected without a native kernel build (CODEGRAPH_KERNEL=0)callersquery that previously returned nothing now returns both true call sitesNot covered
I did not build the Rust kernel, so the kernel path for this change is unvalidated. Both changed functions live in the TS resolution layer above it, but you'll want to confirm that.
Companion PR for the second root cause (alias bindings) is independent of this one and can land in either order; both touch
import-resolver.tsin different regions, so whichever lands second may need a trivial rebase.