Clean up code for nonrelative path completions

Author: Andy Hanson

src/services/pathCompletions.ts

@@ -137,7 +137,6 @@ namespace ts.Completions.PathCompletions {
             if (directories) {
                 for (const directory of directories) {
                     const directoryName = getBaseFileName(normalizePath(directory));
-
                     result.push(nameAndKind(directoryName, ScriptElementKind.directory));
                 }
             }
@@ -177,19 +176,33 @@ namespace ts.Completions.PathCompletions {
             }
         }
 
-        if (compilerOptions.moduleResolution === ModuleResolutionKind.NodeJs) {
-            forEachAncestorDirectory(scriptPath, ancestor => {
-                const nodeModules = combinePaths(ancestor, "node_modules");
-                if (host.directoryExists(nodeModules)) {
-                    getCompletionEntriesForDirectoryFragment(fragment, nodeModules, fileExtensions, /*includeExtensions*/ false, host, /*exclude*/ undefined, result);
-                }
-            });
+        const fragmentDirectory = containsSlash(fragment) ? getDirectoryPath(fragment) : undefined;
+        for (const ambientName of getAmbientModuleCompletions(fragment, fragmentDirectory, typeChecker)) {
+            result.push(nameAndKind(ambientName, ScriptElementKind.externalModuleName));
         }
 
-        getCompletionEntriesFromTypings(host, compilerOptions, scriptPath, result);
-
-        for (const moduleName of enumeratePotentialNonRelativeModules(fragment, scriptPath, compilerOptions, typeChecker, host)) {
-            result.push(nameAndKind(moduleName, ScriptElementKind.externalModuleName));
+        if (getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.NodeJs) {
+            // If looking for a global package name, don't just include everything in `node_modules` because that includes dependencies' own dependencies.
+            // (But do if we didn't find anything, e.g. 'package.json' missing.)
+            let foundGlobal = false;
+            if (fragmentDirectory === undefined) {
+                const oldLength = result.length;
+                getCompletionEntriesFromTypings(host, compilerOptions, scriptPath, result);
+                for (const moduleName of getNamesFromVisibleNodeModules(fragmentDirectory, scriptPath, host)) {
+                    if (!result.some(entry => entry.name === moduleName)) {
+                        result.push(nameAndKind(moduleName, ScriptElementKind.externalModuleName));
+                    }
+                }
+                foundGlobal = result.length !== oldLength;
+            }
+            if (!foundGlobal) {
+                forEachAncestorDirectory(scriptPath, ancestor => {
+                    const nodeModules = combinePaths(ancestor, "node_modules");
+                    if (host.directoryExists(nodeModules)) {
+                        getCompletionEntriesForDirectoryFragment(fragment, nodeModules, fileExtensions, /*includeExtensions*/ false, host, /*exclude*/ undefined, result);
+                    }
+                });
+            }
         }
 
         return result;
@@ -228,7 +241,7 @@ namespace ts.Completions.PathCompletions {
         const normalizedPrefixDirectory = getDirectoryPath(normalizedPrefix);
         const normalizedPrefixBase = getBaseFileName(normalizedPrefix);
 
-        const fragmentHasPath = stringContains(fragment, directorySeparator);
+        const fragmentHasPath = containsSlash(fragment);
 
         // Try and expand the prefix to include any path from the fragment so that we can limit the readDirectory call
         const expandedPrefixDirectory = fragmentHasPath ? combinePaths(normalizedPrefixDirectory, normalizedPrefixBase + getDirectoryPath(fragment)) : normalizedPrefixDirectory;
@@ -262,45 +275,31 @@ namespace ts.Completions.PathCompletions {
         return path[0] === directorySeparator ? path.slice(1) : path;
     }
 
-    function enumeratePotentialNonRelativeModules(fragment: string, scriptPath: string, options: CompilerOptions, typeChecker: TypeChecker, host: LanguageServiceHost): string[] {
-        // Check If this is a nested module
-        const isNestedModule = stringContains(fragment, directorySeparator);
-        const moduleNameFragment = isNestedModule ? fragment.substr(0, fragment.lastIndexOf(directorySeparator)) : undefined;
-
+    function getAmbientModuleCompletions(fragment: string, fragmentDirectory: string | undefined, checker: TypeChecker): ReadonlyArray<string> {
         // Get modules that the type checker picked up
-        const ambientModules = map(typeChecker.getAmbientModules(), sym => stripQuotes(sym.name));
-        let nonRelativeModuleNames = filter(ambientModules, moduleName => startsWith(moduleName, fragment));
+        const ambientModules = checker.getAmbientModules().map(sym => stripQuotes(sym.name));
+        const nonRelativeModuleNames = ambientModules.filter(moduleName => startsWith(moduleName, fragment));
 
         // Nested modules of the form "module-name/sub" need to be adjusted to only return the string
         // after the last '/' that appears in the fragment because that's where the replacement span
         // starts
-        if (isNestedModule) {
-            const moduleNameWithSeperator = ensureTrailingDirectorySeparator(moduleNameFragment);
-            nonRelativeModuleNames = map(nonRelativeModuleNames, nonRelativeModuleName => {
-                return removePrefix(nonRelativeModuleName, moduleNameWithSeperator);
-            });
+        if (fragmentDirectory !== undefined) {
+            const moduleNameWithSeperator = ensureTrailingDirectorySeparator(fragmentDirectory);
+            return nonRelativeModuleNames.map(nonRelativeModuleName => removePrefix(nonRelativeModuleName, moduleNameWithSeperator));
         }
+        return nonRelativeModuleNames;
+    }
 
-
-        if (!options.moduleResolution || options.moduleResolution === ModuleResolutionKind.NodeJs) {
-            for (const visibleModule of enumerateNodeModulesVisibleToScript(host, scriptPath)) {
-                if (!isNestedModule) {
-                    nonRelativeModuleNames.push(visibleModule.moduleName);
-                }
-                else if (startsWith(visibleModule.moduleName, moduleNameFragment)) {
-                    const nestedFiles = tryReadDirectory(host, visibleModule.moduleDir, supportedTypeScriptExtensions, /*exclude*/ undefined, /*include*/ ["./*"]);
-                    if (nestedFiles) {
-                        for (let f of nestedFiles) {
-                            f = normalizePath(f);
-                            const nestedModule = removeFileExtension(getBaseFileName(f));
-                            nonRelativeModuleNames.push(nestedModule);
-                        }
-                    }
-                }
+    function getNamesFromVisibleNodeModules(fragmentDirectory: string | undefined, scriptPath: string, host: LanguageServiceHost): string[] {
+        return flatMap(enumerateNodeModulesVisibleToScript(host, scriptPath), visibleModule => {
+            if (fragmentDirectory === undefined) {
+                return visibleModule.moduleName;
             }
-        }
-
-        return deduplicate(nonRelativeModuleNames, equateStringsCaseSensitive, compareStringsCaseSensitive);
+            else if (startsWith(visibleModule.moduleName, fragmentDirectory)) {
+                const nestedFiles = tryReadDirectory(host, visibleModule.moduleDir, supportedTypeScriptExtensions, /*exclude*/ undefined, /*include*/ ["./*"]);
+                return map(nestedFiles, f => removeFileExtension(getBaseFileName(normalizePath(f))));
+            }
+        });
     }
 
     export function getTripleSlashReferenceCompletion(sourceFile: SourceFile, position: number, compilerOptions: CompilerOptions, host: LanguageServiceHost): ReadonlyArray<PathCompletion> | undefined {
@@ -522,4 +521,8 @@ namespace ts.Completions.PathCompletions {
         catch { /*ignore*/ }
         return undefined;
     }
+
+    function containsSlash(fragment: string) {
+        return stringContains(fragment, directorySeparator);
+    }
 }

tests/cases/fourslash/completionForStringLiteralNonrelativeImport2.ts

@@ -27,11 +27,4 @@
 // @Filename: ambient.ts
 //// declare module "fake-module/other"
 
-const kinds = ["import_as", "import_equals", "require"];
-
-for (const kind of kinds) {
-    goTo.marker(kind + "0");
-    verify.completionListContains("repeated");
-    verify.completionListContains("other");
-    verify.not.completionListItemsCountIsGreaterThan(2);
-}
+verify.completionsAt(["import_as0", "import_equals0", "require0"], ["other", "repeated"], { isNewIdentifierLocation: true })

tests/cases/fourslash/completionForStringLiteralNonrelativeImport3.ts

@@ -27,12 +27,4 @@
 // @Filename: node_modules/fake-module/repeated.jsx
 //// /*repeatedjsx*/
 
-const kinds = ["import_as", "import_equals", "require"];
-
-for (const kind of kinds) {
-    goTo.marker(kind + "0");
-    verify.completionListContains("ts");
-    verify.completionListContains("tsx");
-    verify.completionListContains("dts");
-    verify.not.completionListItemsCountIsGreaterThan(3);
-}
+verify.completionsAt(["import_as0", "import_equals0", "require0"], ["dts", "js", "jsx", "repeated", "ts", "tsx"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionListInImportClause05.ts

@@ -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 });

tests/cases/fourslash/completionsPaths.ts

@@ -23,6 +23,5 @@
 // @Filename: /src/folder/4.ts
 ////const foo = require(`x//*4*/`);
 
-const [r0, r1, r2, r3] = test.ranges();
 verify.completionsAt("1", ["y", "x"], { isNewIdentifierLocation: true });
 verify.completionsAt(["2", "3", "4"], ["bar", "foo"], { isNewIdentifierLocation: true });