Skip to content

Unmangle scoped package names in import completions #21131

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
amcasey merged 9 commits into from
Jan 11, 2018
Merged

Unmangle scoped package names in import completions #21131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fa758cc
Tidy up code to make it harder to call incorrectly
amcasey Jan 9, 2018
db09a59
Unmangle package names from typings during completion
amcasey Jan 9, 2018
e48312d
De-dup typing module completions
amcasey Jan 10, 2018
71c92bf
Fix lint error
amcasey Jan 10, 2018
5d8598f
Add regression test
amcasey Jan 10, 2018
8275bed
Add comment explaining test-only workaround
amcasey Jan 10, 2018
ff102da
Refine explanatory comment
amcasey Jan 10, 2018
211be0a
Add regression test for de-dup'ing.
amcasey Jan 10, 2018
9a4fe8e
Rename getPackageNameFromAtTypesDirectoryWithoutPrefix to getUnmangle…
amcasey Jan 10, 2018
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
11 changes: 8 additions & 3 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,13 +1097,18 @@ namespace ts {
export function getPackageNameFromAtTypesDirectory(mangledName: string): string {
const withoutAtTypePrefix = removePrefix(mangledName, "@types/");
if (withoutAtTypePrefix !== mangledName) {
return stringContains(withoutAtTypePrefix, mangledScopedPackageSeparator) ?
"@" + withoutAtTypePrefix.replace(mangledScopedPackageSeparator, ts.directorySeparator) :
withoutAtTypePrefix;
return getUnmangledNameForScopedPackage(withoutAtTypePrefix);
}
return mangledName;
}

/* @internal */
export function getUnmangledNameForScopedPackage(typesPackageName: string): string {
return stringContains(typesPackageName, mangledScopedPackageSeparator) ?
"@" + typesPackageName.replace(mangledScopedPackageSeparator, ts.directorySeparator) :
typesPackageName;
}

function tryFindNonRelativeModuleNameInCache(cache: PerModuleNameCache | undefined, moduleName: string, containingDirectory: string, traceEnabled: boolean, host: ModuleResolutionHost): SearchResult<Resolved> {
const result = cache && cache.get(containingDirectory);
if (result) {
Expand Down
38 changes: 24 additions & 14 deletions src/services/pathCompletions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,9 +312,11 @@ namespace ts.Completions.PathCompletions {

function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: CompilerOptions, scriptPath: string, span: TextSpan, result: CompletionEntry[] = []): CompletionEntry[] {
// Check for typings specified in compiler options
const seen = createMap<true>();

@ghost ghost Jan 10, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you add a test that fails without this?

@amcasey amcasey Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, I meant to.

if (options.types) {
for (const moduleName of options.types) {
result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span));
for (const typesName of options.types) {
const moduleName = getUnmangledNameForScopedPackage(typesName);
pushResult(moduleName);
}
}
else if (host.getDirectories) {
Expand All @@ -326,32 +328,40 @@ namespace ts.Completions.PathCompletions {

if (typeRoots) {
for (const root of typeRoots) {
getCompletionEntriesFromDirectories(host, root, span, result);
getCompletionEntriesFromDirectories(root);
}
}
}

if (host.getDirectories) {
// Also get all @types typings installed in visible node_modules directories
for (const packageJson of findPackageJsons(scriptPath, host)) {
const typesDir = combinePaths(getDirectoryPath(packageJson), "node_modules/@types");
getCompletionEntriesFromDirectories(host, typesDir, span, result);
getCompletionEntriesFromDirectories(typesDir);
}
}

return result;
}

function getCompletionEntriesFromDirectories(host: LanguageServiceHost, directory: string, span: TextSpan, result: Push<CompletionEntry>) {
if (host.getDirectories && tryDirectoryExists(host, directory)) {
const directories = tryGetDirectories(host, directory);
if (directories) {
for (let typeDirectory of directories) {
typeDirectory = normalizePath(typeDirectory);
result.push(createCompletionEntryForModule(getBaseFileName(typeDirectory), ScriptElementKind.externalModuleName, span));
function getCompletionEntriesFromDirectories(directory: string) {
Debug.assert(!!host.getDirectories);

@ghost ghost Jan 10, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This method is optional -- our test runner will always provide it though, so this assertion won't fail in any tests.

@amcasey amcasey Jan 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure exactly what you're suggestion (remove the assertion?) but I included this assertion because the code was extracted from under an if (host.getDirectories) check. Possibly you're suggesting that that check isn't required either?

@ghost ghost Jan 11, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Whoops, didn't notice that there's still an if, thought it was an assertion now.
Although it looks like tryIOAndConsumeErrors already handles the method being undefined, so the if may be unnecessary anyway.

if (tryDirectoryExists(host, directory)) {
const directories = tryGetDirectories(host, directory);
if (directories) {
for (let typeDirectory of directories) {
typeDirectory = normalizePath(typeDirectory);
const directoryName = getBaseFileName(typeDirectory);
const moduleName = getUnmangledNameForScopedPackage(directoryName);
pushResult(moduleName);
}
}
}
}

function pushResult(moduleName: string) {
if (!seen.has(moduleName)) {
result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span));
seen.set(moduleName, true);
}
}
}

function findPackageJsons(directory: string, host: LanguageServiceHost): string[] {
Expand Down
20 changes: 20 additions & 0 deletions tests/cases/fourslash/completionListInImportClause05.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path='fourslash.ts' />

// @Filename: app.ts
////import * as A from "[|/*1*/|]";

// @Filename: /node_modules/@types/a__b/index.d.ts
////declare module "@e/f" { function fun(): string; }

// @Filename: /node_modules/@types/c__d/index.d.ts
////export declare let x: number;

// NOTE: The node_modules folder is in "/", rather than ".", because it requires
// less scaffolding to mock. In particular, "/" is where we look for type roots.

const [replacementSpan] = test.ranges();
verify.completionsAt("1", [
{ name: "@a/b", replacementSpan },
{ name: "@c/d", replacementSpan },
{ name: "@e/f", replacementSpan },
]);
17 changes: 17 additions & 0 deletions tests/cases/fourslash/completionListInImportClause06.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts' />

// @typeRoots: T1,T2

// @Filename: app.ts
////import * as A from "[|/*1*/|]";

// @Filename: T1/a__b/index.d.ts
////export declare let x: number;

// @Filename: T2/a__b/index.d.ts
////export declare let x: number;

// Confirm that entries are de-dup'd.
verify.completionsAt("1", [
{ name: "@a/b", replacementSpan: test.ranges()[0] },
]);