Fix auto import file extensions with package.json imports wildcards #59564
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| //// external/*exports*/; | ||
|
|
||
| verify.importFixModuleSpecifiers("imports", ["#add.ts"]); | ||
| verify.importFixModuleSpecifiers("exports", ["pkg/external.js"]); |
There was a problem hiding this comment.
Using .ts extensions where possible is limited to "imports", and doesn’t take effect for "exports". We may have to revisit this in the coming years if npm fills up with TS-only packages. We do not currently offer auto-imports for self-name imports (which are resolved through your own package.json "exports"), which is a place where it might make sense to respect allowImportingTsExtensions, so this may need to be revisited in the future as well.
| //// add/*imports*/; | ||
| //// external/*exports*/; | ||
|
|
||
| verify.importFixModuleSpecifiers("imports", ["#add.js"]); |
There was a problem hiding this comment.
This was "#add.d.ts" before this PR 😱
| @@ -980,19 1007,23 @@ function tryGetModuleNameFromExportsOrImports(options: CompilerOptions, host: Mo | |||
| return { moduleFileToTry: combinePaths(packageName, fragment) }; | |||
| } | |||
| if (declarationFile && containsPath(pathOrPattern, declarationFile, ignoreCase)) { | |||
| const fragment = getRelativePathFromDirectory(pathOrPattern, declarationFile, /*ignoreCase*/ false); | |||
| const fragment = changeFullExtension(getRelativePathFromDirectory(pathOrPattern, declarationFile, /*ignoreCase*/ false), getJSExtensionForFile(declarationFile, options)); | |||
There was a problem hiding this comment.
shouldnt this be dependent of file case sensitvity?
There was a problem hiding this comment.
Or the containsPath call should be hard-coded to false, or it might be more complicated than that... pathOrPattern is a concatenation of a path and an "exports" string key, and Node’s resolution algorithm is case sensitive according to my reading. We may be doing this wrong all around.
There was a problem hiding this comment.
paths and case-sensitivity .. ugh..
There was a problem hiding this comment.
Oh wait, it’s an "exports" value, not a key. Yeah, I think the whole thing should use the FS case sensitivity.
There was a problem hiding this comment.
Oh, this possible bug only occurs under MatchingMode.Directory, which was deprecated in Node.js 14 and EOL in 17, so I’m not going to touch this. I added a test for case sensitivity for * patterns and it passed without changes.
There was a problem hiding this comment.
vfs is case insensitive by default. Dont you need to test it on "case sensitive" that its not offered or doesnt do some wierd case replacement.
There was a problem hiding this comment.
The test I added shows that a difference in case between the VFS and the paths written in package.json doesn’t prevent auto-imports from working, which is what’s most important IMO. Also, this PR doesn’t touch/change anything about case sensitivity. This ignoreCase argument was already there.
Fixes #59553
Also discovered and fixed a related bug where auto-imports could give you a
.d.tsextension when resolving through"imports".