Do two passes of exports resolution

andrewbranch · andrewbranch · commit 86eb0702d4c9 · 2021-12-14T12:15:11.000-08:00
diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts
@@ -1600,15 +1600,17 @@ namespace ts {
         entrypoints = append(entrypoints, requireResolution?.path);
 
         if (features & NodeResolutionFeatures.Exports && packageJsonInfo.packageJsonContent.exports) {
-            const exportState = { ...requireState, failedLookupLocations: [], conditions: ["node", "import", "require", "types"] };
-            const exportResolutions = loadEntrypointsFromExportMap(
-                packageJsonInfo,
-                packageJsonInfo.packageJsonContent.exports,
-                exportState,
-                extensions);
-            if (exportResolutions) {
-                for (const resolution of exportResolutions) {
-                    entrypoints = appendIfUnique(entrypoints, resolution.path);
+            for (const conditions of [["node", "import", "types"], ["node", "require", "types"]]) {
+                const exportState = { ...requireState, failedLookupLocations: [], conditions };
+                const exportResolutions = loadEntrypointsFromExportMap(
+                    packageJsonInfo,
+                    packageJsonInfo.packageJsonContent.exports,
+                    exportState,
+                    extensions);
+                if (exportResolutions) {
+                    for (const resolution of exportResolutions) {
+                        entrypoints = appendIfUnique(entrypoints, resolution.path);
+                    }
                 }
             }
         }
diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts
@@ -59,44 +59,55 @@ namespace ts.moduleSpecifiers {
         };
     }
 
+    // `importingSourceFile` and `importingSourceFileName`? Why not just use `importingSourceFile.path`?
+    // Because when this is called by the file renamer, `importingSourceFile` is the file being renamed,
+    // while `importingSourceFileName` its *new* name. We need a source file just to get its
+    // `impliedNodeFormat` and to detect certain preferences from existing import module specifiers.
     export function updateModuleSpecifier(
         compilerOptions: CompilerOptions,
+        importingSourceFile: SourceFile,
         importingSourceFileName: Path,
         toFileName: string,
         host: ModuleSpecifierResolutionHost,
         oldImportSpecifier: string,
     ): string | undefined {
-        const res = getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferencesForUpdate(compilerOptions, oldImportSpecifier, importingSourceFileName, host), {});
+        const res = getModuleSpecifierWorker(compilerOptions, importingSourceFile, importingSourceFile.path, toFileName, host, getPreferencesForUpdate(compilerOptions, oldImportSpecifier, importingSourceFileName, host), {});
         if (res === oldImportSpecifier) return undefined;
         return res;
     }
 
-    // Note: importingSourceFile is just for usesJsExtensionOnImports
+    // `importingSourceFile` and `importingSourceFileName`? Why not just use `importingSourceFile.path`?
+    // Because when this is called by the declaration emitter, `importingSourceFile` is the implementation
+    // file, but `importingSourceFileName` and `toFileName` refer to declaration files (the former to the
+    // one currently being produced; the latter to the one being imported). We need an implementation file
+    // just to get its `impliedNodeFormat` and to detect certain preferences from existing import module
+    // specifiers.
     export function getModuleSpecifier(
         compilerOptions: CompilerOptions,
         importingSourceFile: SourceFile,
         importingSourceFileName: Path,
         toFileName: string,
         host: ModuleSpecifierResolutionHost,
     ): string {
-        return getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferences(host, {}, compilerOptions, importingSourceFile), {});
+        return getModuleSpecifierWorker(compilerOptions, importingSourceFile, importingSourceFileName, toFileName, host, getPreferences(host, {}, compilerOptions, importingSourceFile), {});
     }
 
     export function getNodeModulesPackageName(
         compilerOptions: CompilerOptions,
-        importingSourceFileName: Path,
+        importingSourceFile: SourceFile,
         nodeModulesFileName: string,
         host: ModuleSpecifierResolutionHost,
         preferences: UserPreferences,
     ): string | undefined {
-        const info = getInfo(importingSourceFileName, host);
-        const modulePaths = getAllModulePaths(importingSourceFileName, nodeModulesFileName, host, preferences);
+        const info = getInfo(importingSourceFile.path, host);
+        const modulePaths = getAllModulePaths(importingSourceFile.path, nodeModulesFileName, host, preferences);
         return firstDefined(modulePaths,
-            modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions, /*packageNameOnly*/ true));
+            modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, /*packageNameOnly*/ true));
     }
 
     function getModuleSpecifierWorker(
         compilerOptions: CompilerOptions,
+        importingSourceFile: SourceFile,
         importingSourceFileName: Path,
         toFileName: string,
         host: ModuleSpecifierResolutionHost,
@@ -105,7 +116,7 @@ namespace ts.moduleSpecifiers {
     ): string {
         const info = getInfo(importingSourceFileName, host);
         const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host, userPreferences);
-        return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions)) ||
+        return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions)) ||
             getLocalModuleSpecifier(toFileName, info, compilerOptions, host, preferences);
     }
 
@@ -222,7 +233,7 @@ namespace ts.moduleSpecifiers {
         let pathsSpecifiers: string[] | undefined;
         let relativeSpecifiers: string[] | undefined;
         for (const modulePath of modulePaths) {
-            const specifier = tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions);
+            const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions);
             nodeModulesSpecifiers = append(nodeModulesSpecifiers, specifier);
             if (specifier && modulePath.isRedirect) {
                 // If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
@@ -639,7 +650,7 @@ namespace ts.moduleSpecifiers {
             : removeFileExtension(relativePath);
     }
 
-    function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean): string | undefined {
+    function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, importingSourceFile: SourceFile , host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean): string | undefined {
         if (!host.fileExists || !host.readFile) {
             return undefined;
         }
@@ -706,10 +717,14 @@ namespace ts.moduleSpecifiers {
             let moduleFileToTry = path;
             if (host.fileExists(packageJsonPath)) {
                 const packageJsonContent = JSON.parse(host.readFile!(packageJsonPath)!);
-                // TODO: Inject `require` or `import` condition based on the intended import mode
                 if (getEmitModuleResolutionKind(options) === ModuleResolutionKind.Node12 || getEmitModuleResolutionKind(options) === ModuleResolutionKind.NodeNext) {
+                    // `conditions` *could* be made to go against `importingSourceFile.impliedNodeFormat` if something wanted to generate
+                    // an ImportEqualsDeclaration in an ESM-implied file or an ImportCall in a CJS-implied file. But since this function is
+                    // usually called to conjure an import out of thin air, we don't have an existing usage to call `getModeForUsageAtIndex`
+                    // with, so for now we just stick with the mode of the file.
+                    const conditions = ["node", importingSourceFile.impliedNodeFormat === ModuleKind.ESNext ? "import" : "require", "types"];
                     const fromExports = packageJsonContent.exports && typeof packageJsonContent.name === "string"
-                        ? tryGetModuleNameFromExports(options, path, packageRootPath, getPackageNameFromTypesPackageName(packageJsonContent.name), packageJsonContent.exports, ["node", "types"])
+                        ? tryGetModuleNameFromExports(options, path, packageRootPath, getPackageNameFromTypesPackageName(packageJsonContent.name), packageJsonContent.exports, conditions)
                         : undefined;
                     if (fromExports) {
                         const withJsExtension = !hasTSFileExtension(fromExports.moduleFileToTry)
diff --git a/src/services/getEditsForFileRename.ts b/src/services/getEditsForFileRename.ts
@@ -156,7 +156,7 @@ namespace ts {
 
                     // Need an update if the imported file moved, or the importing file moved and was using a relative path.
                     return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text)))
-                        ? moduleSpecifiers.updateModuleSpecifier(program.getCompilerOptions(), getCanonicalFileName(newImportFromPath) as Path, toImport.newFileName, createModuleSpecifierResolutionHost(program, host), importLiteral.text)
+                        ? moduleSpecifiers.updateModuleSpecifier(program.getCompilerOptions(), sourceFile, getCanonicalFileName(newImportFromPath) as Path, toImport.newFileName, createModuleSpecifierResolutionHost(program, host), importLiteral.text)
                         : undefined;
                 });
         }
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -3085,7 +3085,7 @@ namespace ts {
             }
             const specifier = moduleSpecifiers.getNodeModulesPackageName(
                 host.getCompilationSettings(),
-                fromFile.path,
+                fromFile,
                 importedFileName,
                 moduleSpecifierResolutionHost,
                 preferences,
diff --git a/tests/cases/fourslash/server/autoImportProvider_exportMap8.ts b/tests/cases/fourslash/server/autoImportProvider_exportMap8.ts
@@ -0,0 +1,94 @@
+/// <reference path="../fourslash.ts"/>
+
+// Both 'import' and 'require' should be pulled in
+
+// @Filename: /tsconfig.json
+//// {
+////   "compilerOptions": {
+////     "module": "nodenext"
+////   }
+//// }
+
+// @Filename: /package.json
+//// {
+////   "type": "module",
+////   "dependencies": {
+////     "dependency": "^1.0.0"
+////   }
+//// }
+
+// @Filename: /node_modules/dependency/package.json
+//// {
+////   "type": "module",
+////   "name": "dependency",
+////   "version": "1.0.0",
+////   "exports": {
+////     "./lol": {
+////       "import": "./lib/index.js",
+////       "require": "./lib/lol.js"
+////     }
+////   }
+//// }
+
+// @Filename: /node_modules/dependency/lib/index.d.ts
+//// export function fooFromIndex(): void;
+
+// @Filename: /node_modules/dependency/lib/lol.d.ts
+//// export function fooFromLol(): void;
+
+// @Filename: /src/bar.ts
+//// import { fooFromIndex } from "dependency";
+
+// @Filename: /src/foo.cts
+//// fooFrom/*cts*/
+
+// @Filename: /src/foo.mts
+//// fooFrom/*mts*/
+
+goTo.marker("cts");
+verify.completions({
+  marker: "cts",
+  exact: completion.globalsPlus([{
+    // TODO: this one will go away (see note in ./autoImportProvider_exportMap3.ts)
+    name: "fooFromIndex",
+    source: "../node_modules/dependency/lib/index",
+    sourceDisplay: "../node_modules/dependency/lib/index",
+    sortText: completion.SortText.AutoImportSuggestions,
+    hasAction: true,
+  }, {
+    name: "fooFromLol",
+    source: "dependency/lol",
+    sourceDisplay: "dependency/lol",
+    sortText: completion.SortText.AutoImportSuggestions,
+    hasAction: true,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    includeInsertTextCompletions: true,
+    allowIncompleteCompletions: true,
+  },
+});
+
+// goTo.marker("mts");
+// verify.completions({
+//   marker: "mts",
+//   exact: completion.globalsPlus([{
+//     name: "fooFromIndex",
+//     source: "dependency/lol",
+//     sourceDisplay: "dependency/lol",
+//     sortText: completion.SortText.AutoImportSuggestions,
+//     hasAction: true,
+//   }, {
+//     // TODO: this one will go away (see note in ./autoImportProvider_exportMap3.ts)
+//     name: "fooFromLol",
+//     source: "../node_modules/dependency/lib/index.js",
+//     sourceDisplay: "../node_modules/dependency/lib/index.js",
+//     sortText: completion.SortText.AutoImportSuggestions,
+//     hasAction: true,
+//   }]),
+//   preferences: {
+//     includeCompletionsForModuleExports: true,
+//     includeInsertTextCompletions: true,
+//     allowIncompleteCompletions: true,
+//   },
+// });

Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ namespace ts {`
`156`	`156`
`157`	`157`	`// Need an update if the imported file moved, or the importing file moved and was using a relative path.`
`158`	`158`	`return toImport !== undefined && (toImport.updated \|\| (importingSourceFileMoved && pathIsRelative(importLiteral.text)))`
`159`		`- ? moduleSpecifiers.updateModuleSpecifier(program.getCompilerOptions(), getCanonicalFileName(newImportFromPath) as Path, toImport.newFileName, createModuleSpecifierResolutionHost(program, host), importLiteral.text)`
	`159`	`+ ? moduleSpecifiers.updateModuleSpecifier(program.getCompilerOptions(), sourceFile, getCanonicalFileName(newImportFromPath) as Path, toImport.newFileName, createModuleSpecifierResolutionHost(program, host), importLiteral.text)`
`160`	`160`	`: undefined;`
`161`	`161`	`});`
`162`	`162`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3085,7 +3085,7 @@ namespace ts {`
`3085`	`3085`	`}`
`3086`	`3086`	`const specifier = moduleSpecifiers.getNodeModulesPackageName(`
`3087`	`3087`	`host.getCompilationSettings(),`
`3088`		`- fromFile.path,`
	`3088`	`+ fromFile,`
`3089`	`3089`	`importedFileName,`
`3090`	`3090`	`moduleSpecifierResolutionHost,`
`3091`	`3091`	`preferences,`