Let AutoImportProvider find non-declaration files in wildcard exports... #56848
Conversation
| Info seq [hh:mm:ss:mss] Files (5) | ||
| Info seq [hh:mm:ss:mss] Files (6) | ||
| /node_modules/pkg/a/a1.d.ts Text-1 "export const a1: number;" | ||
| /node_modules/pkg/b/b1.d.ts Text-1 "export const b1: number;" | ||
| /node_modules/pkg/b/b2.d.mts Text-1 "export const NOT_REACHABLE: number;" |
There was a problem hiding this comment.
I guess I could have kept the same strict extension filtering while mixing in the allowed TS implementation file extensions, keeping this unreachable file out of the AutoImportProviderProject compilation. But it doesn’t seem realistic for packages to ship declaration files that are literally unreachable, so doing the simpler thing and allowing all supported extensions seems fine. Updated the test to show that this file doesn’t result in a new auto-import.
src/compiler/moduleNameResolver.ts
Outdated
| @@ -2292,8 +2291,8 @@ function loadEntrypointsFromExportMap( | |||
| /*excludes*/ undefined, | |||
| [ | |||
| isDeclarationFileName(target) | |||
| ? replaceFirstStar(target, "**/*") | |||
| : changeAnyExtension(replaceFirstStar(target, "**/*"), getDeclarationEmitExtensionForPath(target)), | |||
| ? changeAnyExtension(replaceFirstStar(target, "**/*"), ".*", supportedDeclarationExtensions, /*ignoreCase*/ true) | |||
There was a problem hiding this comment.
It seems like we are only filtering out replacing to *.* only if extension is of type ".d.someext.ts"
Was that intentional. I wonder if we can just always do changeAnyExtension(replaceFirstStar(target, "**/*"), ".*") irrespective of extension .
There was a problem hiding this comment.
The problem is changeAnyExtension(path, ".*") only replaces from the last ., so that would result in foo.d.ts becoming foo.d.*, when we need it to become foo.* instead.
There was a problem hiding this comment.
but if i locally change this to do changeAnyExtension(replaceFirstStar(target, "**/*"), ".*"), none of the tests fail ?
There was a problem hiding this comment.
You're right. There's currently no test where the exports target has a declaration file extension but we find a .ts on disk instead. While that should arguably work, it's not very realistic.
There was a problem hiding this comment.
Presumably we don't have many tests with export maps pointing explicitly at declaration files with js or implementation files alongside them that we'd prefer to lookup - it's usually the other way around, so only finding matches for name.d.* gets us what we want almost all the time.
There was a problem hiding this comment.
Yeah, exactly. Still, I running with an include of name.d.* feels too much like a mistake to me. I made a new helper changeFullExtension that recognizes and swaps all declaration file extensions (including ones for arbitrary file extensions, which the current code doesn't handle correctly), and am just trying to figure out how to push the commit I made on my host machine from my dev box 😅
src/compiler/path.ts
Outdated
| export function changeFullExtension(path: string, newExtension: string) { | ||
| const declarationExtension = getDeclarationFileExtension(path); | ||
| if (declarationExtension) { | ||
| return path.slice(0, path.length - declarationExtension.length) + newExtension; |
There was a problem hiding this comment.
Does this need to handle presence or absence of "." in new extension like in changeAnyExtension : (startsWith(ext, ".") ? ext : "." + ext)
...which is important for internal packages consumed by bundlers in monorepos, which typically point their package.json entries to TS implementation files.
Fixes #53116 (comment)