Clean up code for nonrelative path completions #23150
Conversation
| @@ -12,4 +12,4 @@ | |||
| // 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. | |||
|
|
|||
| verify.completionsAt("1", ["@a/b", "@c/d", "@e/f"], { isNewIdentifierLocation: true }); | |||
| verify.completionsAt("1", ["@e/f", "@a/b", "@c/d"], { isNewIdentifierLocation: true }); | |||
There was a problem hiding this comment.
The order changed because I moved up the check for ambient module declarations. Don't think it matters.
There was a problem hiding this comment.
Shouldn't the results be sorted?
There was a problem hiding this comment.
They weren't sorted by name before, except by coincidence. We could make a PR sorting them but it would change a bunch of baselines. I think editors do their own sorting on the result anyway.
src/services/pathCompletions.ts
Outdated
| } | ||
| function getNamesFromVisibleNodeModules(fragmentDirectory: string | undefined, scriptPath: string, host: LanguageServiceHost): string[] { | ||
| return flatMap(enumerateNodeModulesVisibleToScript(host, scriptPath), visibleModule => { | ||
| if (fragmentDirectory === undefined) { |
There was a problem hiding this comment.
Isnt this always undefined since its called only when fragmentDirectory ===undefined
There was a problem hiding this comment.
Wow, if we're always in the first case we can clean out a lot of dead code.
ghost
force-pushed
the
pathCompletionsNonrelative
branch
from
5e80112 to
be6e420
Compare
src/services/pathCompletions.ts
Outdated
| if (!foundGlobal) { | ||
| forEachAncestorDirectory(scriptPath, ancestor => { | ||
| const nodeModules = combinePaths(ancestor, "node_modules"); | ||
| if (host.directoryExists(nodeModules)) { |
There was a problem hiding this comment.
host.directoryExists is optional, right?
| @@ -117,9 +117,10 @@ namespace ts { | |||
| } | |||
| } | |||
|
|
|||
| function readJson(path: string, host: ModuleResolutionHost): PackageJson { | |||
| /* @internal */ | |||
| export function readJson(path: string, host: { readFile(fileName: string): string | undefined }): object { | |||
There was a problem hiding this comment.
Why not mark readFile optional?
There was a problem hiding this comment.
Whoops, left in a change to the function body on accident. This should not be optional here because it is checked by the callers.
src/services/pathCompletions.ts
Outdated
| let foundGlobal = false; | ||
| if (fragmentDirectory === undefined) { | ||
| const oldLength = result.length; | ||
| getCompletionEntriesFromTypings(host, compilerOptions, scriptPath, result); |
There was a problem hiding this comment.
earlier this was done irrespective of fragment/module emit .. Why the change?
|
@weswigham Good to go? |
Fixes #23096
moduleResolutionKindin one place. And correctly check it usinggetEmitModuleResolutionKind.package.jsonand add completions from searching throughnode_modules. Now, we will only do the latter if the former failed.moduleResolutionKind.