Skip to content

importModuleSpecifierEnding changes .ts string completions to .js #44602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 21, 2021
+95 −15
Merged

importModuleSpecifierEnding changes .ts string completions to .js #44602

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc80712
don't add .ts extensions to imports
jessetrinity Jun 15, 2021
76509c3
Update src/services/stringCompletions.ts
Jun 15, 2021
25555c3
add other supported extension types
jessetrinity Jun 21, 2021
ab950c0
add final newlines
jessetrinity Jun 21, 2021
5adaabc
adress PR comment
jessetrinity Jun 21, 2021
15a4735
add unsupported extension test
jessetrinity Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,13 +687,17 @@ namespace ts.moduleSpecifiers {
case Ending.Index:
return noExtension;
case Ending.JsExtension:
return noExtension + getJSExtensionForFile(fileName, options);
return noExtension + getJSExtensionForFileIfApplicable(fileName, options);
default:
return Debug.assertNever(ending);
}
}

function getJSExtensionForFile(fileName: string, options: CompilerOptions): Extension {
function getJSExtensionForFileIfApplicable(fileName: string, options: CompilerOptions): Extension {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return getJSExtensionForFile(fileName, options) ?? Debug.fail(`Extension ${extensionFromPath(fileName)} is unsupported:: FileName:: ${fileName}`);
}

export function getJSExtensionForFile(fileName: string, options: CompilerOptions): Extension | undefined {
const ext = extensionFromPath(fileName);
Copy link
Member

@andrewbranch andrewbranch Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could certainly be seeing an artifact of the fourslash client 😬. Have you checked the behavior against a Real Editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was pretty sure that tryReadDirectory only returned files that matched its extensions argument, if provided. I might be misreading though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either way, updated to not use the throwing variant :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh geez TIL. Is that a wildcard module declaration?

switch (ext) {
case Extension.Ts:
Expand All @@ -705,10 +709,8 @@ namespace ts.moduleSpecifiers {
case Extension.Jsx:
case Extension.Json:
return ext;
case Extension.TsBuildInfo:
return Debug.fail(`Extension ${Extension.TsBuildInfo} is unsupported:: FileName:: ${fileName}`);
default:
return Debug.assertNever(ext);
return undefined;
}
}

Expand Down
36 changes: 27 additions & 9 deletions src/services/stringCompletions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,14 @@ namespace ts.Completions.StringCompletions {

interface ExtensionOptions {
readonly extensions: readonly Extension[];
readonly includeExtensions: boolean;
readonly includeExtensionsOption: IncludeExtensionsOption;
}
function getExtensionOptions(compilerOptions: CompilerOptions, includeExtensions = false): ExtensionOptions {
return { extensions: getSupportedExtensionsForModuleResolution(compilerOptions), includeExtensions };
function getExtensionOptions(compilerOptions: CompilerOptions, includeExtensionsOption = IncludeExtensionsOption.Exclude): ExtensionOptions {
return { extensions: getSupportedExtensionsForModuleResolution(compilerOptions), includeExtensionsOption };
}
function getCompletionEntriesForRelativeModules(literalValue: string, scriptDirectory: string, compilerOptions: CompilerOptions, host: LanguageServiceHost, scriptPath: Path, preferences: UserPreferences) {
const extensionOptions = getExtensionOptions(compilerOptions, preferences.importModuleSpecifierEnding === "js");
const includeExtensions = preferences.importModuleSpecifierEnding === "js" ? IncludeExtensionsOption.ModuleSpecifierCompletion : IncludeExtensionsOption.Exclude;
const extensionOptions = getExtensionOptions(compilerOptions, includeExtensions);
if (compilerOptions.rootDirs) {
return getCompletionEntriesForDirectoryFragmentWithRootDirs(
compilerOptions.rootDirs, literalValue, scriptDirectory, extensionOptions, compilerOptions, host, scriptPath);
Expand Down Expand Up @@ -370,10 +371,15 @@ namespace ts.Completions.StringCompletions {
return flatMap(baseDirectories, baseDirectory => getCompletionEntriesForDirectoryFragment(fragment, baseDirectory, extensionOptions, host, exclude));
}

const enum IncludeExtensionsOption {
Exclude,
Include,
ModuleSpecifierCompletion,
}
/**
* Given a path ending at a directory, gets the completions for the path, and filters for those entries containing the basename.
*/
function getCompletionEntriesForDirectoryFragment(fragment: string, scriptPath: string, { extensions, includeExtensions }: ExtensionOptions, host: LanguageServiceHost, exclude?: string, result: NameAndKind[] = []): NameAndKind[] {
function getCompletionEntriesForDirectoryFragment(fragment: string, scriptPath: string, { extensions, includeExtensionsOption }: ExtensionOptions, host: LanguageServiceHost, exclude?: string, result: NameAndKind[] = []): NameAndKind[] {
if (fragment === undefined) {
fragment = "";
}
Expand Down Expand Up @@ -407,7 +413,7 @@ namespace ts.Completions.StringCompletions {
if (files) {
/**
* Multiple file entries might map to the same truncated name once we remove extensions
* (happens iff includeExtensions === false)so we use a set-like data structure. Eg:
* (happens iff includeExtensionsOption === includeExtensionsOption.Exclude) so we use a set-like data structure. Eg:
*
* both foo.ts and foo.tsx become foo
*/
Expand All @@ -418,8 +424,20 @@ namespace ts.Completions.StringCompletions {
continue;
}

const foundFileName = includeExtensions || fileExtensionIs(filePath, Extension.Json) ? getBaseFileName(filePath) : removeFileExtension(getBaseFileName(filePath));
foundFiles.set(foundFileName, tryGetExtensionFromPath(filePath));
let foundFileName: string;
const outputExtension = moduleSpecifiers.getJSExtensionForFile(filePath, host.getCompilationSettings());
if (includeExtensionsOption === IncludeExtensionsOption.Exclude && !fileExtensionIs(filePath, Extension.Json)) {
foundFileName = removeFileExtension(getBaseFileName(filePath));
foundFiles.set(foundFileName, tryGetExtensionFromPath(filePath));
}
else if (includeExtensionsOption === IncludeExtensionsOption.ModuleSpecifierCompletion && outputExtension) {
foundFileName = changeExtension(getBaseFileName(filePath), outputExtension);
foundFiles.set(foundFileName, outputExtension);
}
else {
foundFileName = getBaseFileName(filePath);
foundFiles.set(foundFileName, tryGetExtensionFromPath(filePath));
}
}

foundFiles.forEach((ext, foundFile) => {
Expand Down Expand Up @@ -635,7 +653,7 @@ namespace ts.Completions.StringCompletions {

const [, prefix, kind, toComplete] = match;
const scriptPath = getDirectoryPath(sourceFile.path);
const names = kind === "path" ? getCompletionEntriesForDirectoryFragment(toComplete, scriptPath, getExtensionOptions(compilerOptions, /*includeExtensions*/ true), host, sourceFile.path)
const names = kind === "path" ? getCompletionEntriesForDirectoryFragment(toComplete, scriptPath, getExtensionOptions(compilerOptions, IncludeExtensionsOption.Include), host, sourceFile.path)
: kind === "types" ? getCompletionEntriesFromTypings(host, compilerOptions, scriptPath, getFragmentDirectory(toComplete), getExtensionOptions(compilerOptions))
: Debug.fail();
return addReplacementSpans(toComplete, range.pos + prefix.length, names);
Expand Down
9 changes: 9 additions & 0 deletions tests/cases/fourslash/completionImportModuleSpecifierEndingDts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/// <reference path='fourslash.ts'/>
//@Filename:test.d.ts
//// export declare class Test {}

//@Filename:module.ts
////import { Test } from ".//**/"

verify.completions({ marker: "", includes:{name:"test.js"}, preferences: {importModuleSpecifierEnding: "js" }, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});
4 changes: 2 additions & 2 deletions .../completionImportModuleSpecifierEnding.ts → ...ompletionImportModuleSpecifierEndingJs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
//@Filename:module.js
////import { f } from ".//**/"


verify.completions({ marker: "", includes:{name:"test.js"}, preferences: {importModuleSpecifierEnding: "js" }, isNewIdentifierLocation: true})
verify.completions({ marker: "", includes:{name:"test.js"}, preferences: {importModuleSpecifierEnding: "js" }, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});
11 changes: 11 additions & 0 deletions tests/cases/fourslash/completionImportModuleSpecifierEndingJsx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts'/>
//@allowJs: true
//@jsx:preserve
//@Filename:test.jsx
//// export class Test { }

//@Filename:module.jsx
////import { Test } from ".//**/"

verify.completions({ marker: "", includes:{name:"test.jsx"}, preferences: {importModuleSpecifierEnding: "js"}, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});
11 changes: 11 additions & 0 deletions tests/cases/fourslash/completionImportModuleSpecifierEndingTs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts'/>
//@Filename:test.ts
////export function f(){
//// return 1
////}

//@Filename:module.ts
////import { f } from ".//**/"

verify.completions({ marker: "", includes:{name:"test.js"}, preferences: {importModuleSpecifierEnding: "js" }, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});
10 changes: 10 additions & 0 deletions tests/cases/fourslash/completionImportModuleSpecifierEndingTsxPreserve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path='fourslash.ts'/>
//@jsx:preserve
//@Filename:test.tsx
//// export class Test { }

//@Filename:module.tsx
////import { Test } from ".//**/"

verify.completions({ marker: "", includes:{name:"test.jsx"}, preferences: {importModuleSpecifierEnding: "js"}, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});
10 changes: 10 additions & 0 deletions tests/cases/fourslash/completionImportModuleSpecifierEndingTsxReact.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path='fourslash.ts'/>
//@jsx:react
//@Filename:test.tsx
//// export class Test { }

//@Filename:module.tsx
////import { Test } from ".//**/"

verify.completions({ marker: "", includes:{name:"test.js"}, preferences: {importModuleSpecifierEnding: "js"}, isNewIdentifierLocation: true});
verify.completions({ marker: "", includes:{name:"test"}, preferences: {importModuleSpecifierEnding: "index" }, isNewIdentifierLocation: true});