importModuleSpecifierEnding changes .ts string completions to .js #44602
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
jessetrinity
changed the title
src/services/stringCompletions.ts
Outdated
| foundFileName = removeFileExtension(getBaseFileName(filePath)); | ||
| foundFiles.set(foundFileName, tryGetExtensionFromPath(filePath)); | ||
| } | ||
| else if (includeExtensionsOption === IncludeExtensionsOption.ModuleSpecifierCompletion && fileExtensionIs(filePath, Extension.Ts)) { |
There was a problem hiding this comment.
What we really need, and I’m not sure whether we have (but we might), is a function that maps input file extensions to emitted file extensions. So we would have
- .ts → .js
- .tsx → ? (I don’t know if this is always .js or if we might emit .jsx with
--jsx=preserve?) - .mts → .mjs (someday soon)
- .d.ts →
undefinedbecause it doesn’t get emitted - .json → .json (only if
--resolveJsonModule?) - .js → .js
If we don’t have that function, we need to (a) figure out what the right thing for TSX is and implement it now, and (b) remember to update it for .mts and .cts if/when we add those.
There was a problem hiding this comment.
Emitter.ts has getOutputExtension which does approximately that, but it works on a sourceFile. Since we're just working working with the paths here we probably just want a new similar function that handle the cases we need.
There was a problem hiding this comment.
👍 in absence of something better, just leave comments on each function (getOutputExtension and the new function) saying to keep them in sync with each other.
There was a problem hiding this comment.
Found it:
TypeScript/src/compiler/moduleSpecifiers.ts
Line 696 in fad9122
I would maybe just export that function so it doesn’t immediately cause merge conflicts with #44501.
There was a problem hiding this comment.
Actually, you might need to refactor it to return undefined instead of throwing on unsupported extensions, and move the debug assertions to its caller, or another wrapped version of it. But otherwise I think this function is exactly the thing we were looking for.
Co-authored-by: Andrew Branch <[email protected]>
src/compiler/moduleSpecifiers.ts
Outdated
| } | ||
|
|
||
| function getJSExtensionForFile(fileName: string, options: CompilerOptions): Extension { | ||
| function getJSExtensionForFileIfApplicable(fileName: string, options: CompilerOptions): Extension { |
There was a problem hiding this comment.
To me this one sounds like the one that never throws. I would suggest naming the undefined-returning one tryGetJSExtensionForFile and the throwing one getJSExtensionForFile.
src/compiler/moduleSpecifiers.ts
Outdated
| } | ||
|
|
||
| export function getJSExtensionForFile(fileName: string, options: CompilerOptions): Extension | undefined { | ||
| const ext = extensionFromPath(fileName); |
There was a problem hiding this comment.
I thought it was sketchy that this function didn’t have undefined in its return type, so I looked and found that it too has an undefined-returning counterpart, and this one throws! This may need to be swapped for tryGetExtensionFromPath for good measure. This also makes me realize we should have a test that actually returns completions for a non-source-file file extension, like index.css or something.
There was a problem hiding this comment.
I don't think we even read other extension types from disk, but I think you mean something like
//@filename: index.css
body {}
//@filename: index.html
<!DOCTYPE html>
//@filename: foo.ts
import "./And expect nothing at the uncompleted string?
There was a problem hiding this comment.
Hm, I looked at stringCompletions and saw tryReadDirectory, so I assumed we were offering everything we could find, with extension substitutions as appropriate. Not sure what’s going on then 🤔
There was a problem hiding this comment.
You could certainly be seeing an artifact of the fourslash client 😬. Have you checked the behavior against a Real Editor?
There was a problem hiding this comment.
I was pretty sure that tryReadDirectory only returned files that matched its extensions argument, if provided. I might be misreading though.
There was a problem hiding this comment.
either way, updated to not use the throwing variant :)
There was a problem hiding this comment.
Ah, I missed that parameter 🤦♂️
It might actually be nice to allow all extensions here in the future, since we can no longer seriously claim to believe we’re in a world where people only import things we know how to analyze. This doesn’t even account for declare module "*.css" pattern ambient module declarations. But that’s for another day. Thanks!
There was a problem hiding this comment.
Oh geez TIL. Is that a wildcard module declaration?
3 participants
Fixes #44374