Unmangle scoped package names in import completions#21131
Conversation
| for (const moduleName of options.types) { | ||
| result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); | ||
| for (const typesName of options.types) { | ||
| const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(typesName); |
There was a problem hiding this comment.
True, but I found it helpful for debugging.
There was a problem hiding this comment.
If debugging in chrome you can actually see the return value before exiting a function even if it's not named. That's in "Scope > Local > Return value" in the right-hand menu.
| typeDirectory = normalizePath(typeDirectory); | ||
| result.push(createCompletionEntryForModule(getBaseFileName(typeDirectory), ScriptElementKind.externalModuleName, span)); | ||
| function getCompletionEntriesFromDirectories(directory: string) { | ||
| Debug.assert(!!host.getDirectories); |
There was a problem hiding this comment.
This method is optional -- our test runner will always provide it though, so this assertion won't fail in any tests.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // @Filename: /node_modules/@types/c__d/index.d.ts | ||
| ////export declare let x: number; | ||
|
|
||
| // NOTE: When performing completion, the "current directory" appears to be "/", |
There was a problem hiding this comment.
I think this may be a correct behavior -- host.getCurrentDirectory() returns process.cwd();, not necessarily the root of your project.
There was a problem hiding this comment.
I've refined the comment.
| } | ||
|
|
||
| /* @internal */ | ||
| export function getPackageNameFromAtTypesDirectoryWithoutPrefix(withoutAtTypePrefix: string): string { |
There was a problem hiding this comment.
I would call this unmangleTypesPackageName, and the parameter typesPackageName.
There was a problem hiding this comment.
I went with getUnmangledNameForScopedPackage for consistency with getMangledNameForScopedPackage.
|
|
||
| function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: CompilerOptions, scriptPath: string, span: TextSpan, result: CompletionEntry[] = []): CompletionEntry[] { | ||
| // Check for typings specified in compiler options | ||
| const seen = createMap<true>(); |
There was a problem hiding this comment.
Could you add a test that fails without this?
"@" is only allowed at the start of a package name, so the types for package
@a/bare put in@types/a__b. We want completion to offer@a/b, rather thana__b.Note that, in at least some cases, it is impossible to determine whether
@types/a__bcame from@a/bora__b. We don't try to distinguish - we always offer@a/b. (Conceivably, we could switch toa__bif it resolves and@a/bdoes not but, at present, there are no typings for packages with "__" in the name.)Fixes #15533