Skip to content
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

Offer auto-imports from wildcard exports with AutoImportProvider #54831

Merged
merged 4 commits into from
Jul 7, 2023
Merged

Offer auto-imports from wildcard exports with AutoImportProvider #54831

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fc2d7d
Find wildcard exports with AutoImportProvider
andrewbranch Jun 29, 2023
77267b5
Update API baseline
andrewbranch Jun 29, 2023
04c06ee
Fix conditional wildcard module specifier generation bug
andrewbranch Jun 29, 2023
7888c56
Merge branch 'main' into feature/53116
andrewbranch Jul 3, 2023
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
84 changes: 61 additions & 23 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
CommandLineOption,
comparePaths,
Comparison,
CompilerHost,
CompilerOptions,
concatenate,
contains,
Expand All @@ -35,10 +36,12 @@ import {
forEach,
forEachAncestorDirectory,
formatMessage,
getAnyExtensionFromPath,
getBaseFileName,
GetCanonicalFileName,
getCommonSourceDirectory,
getCompilerOptionValue,
getDeclarationEmitExtensionForPath,
getDirectoryPath,
GetEffectiveTypeRootsHost,
getEmitModuleKind,
Expand Down Expand Up @@ -98,6 +101,7 @@ import {
startsWith,
stringContains,
supportedDeclarationExtensions,
supportedJSExtensionsFlat,
supportedTSImplementationExtensions,
toPath,
tryExtractTSExtension,
Expand Down Expand Up @@ -193,6 +197,15 @@ function formatExtensions(extensions: Extensions) {
return result.join(", ");
}

function extensionsToExtensionsArray(extensions: Extensions) {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, how often does this happen? Is it worth caching in some way given the key is just a number?

Copy link
Member Author

Choose a reason for hiding this comment

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

The number of packages AutoImportProvider will process is capped at 10, so surely not enough times to be make an observable impact.

const result: Extension[] = [];
if (extensions & Extensions.TypeScript) result.push(...supportedTSImplementationExtensions);
if (extensions & Extensions.JavaScript) result.push(...supportedJSExtensionsFlat);
if (extensions & Extensions.Declaration) result.push(...supportedDeclarationExtensions);
if (extensions & Extensions.Json) result.push(Extension.Json);
return result;
}

interface PathAndPackageId {
readonly fileName: string;
readonly packageId: PackageId | undefined;
Expand Down Expand Up @@ -2084,11 +2097,16 @@ function loadNodeModuleFromDirectory(extensions: Extensions, candidate: string,
return withPackageId(packageInfo, loadNodeModuleFromDirectoryWorker(extensions, candidate, onlyRecordFailures, state, packageJsonContent, versionPaths));
}

/** @internal */
export interface GetPackageJsonEntrypointsHost extends ModuleResolutionHost {
readDirectory: CompilerHost["readDirectory"];
}

/** @internal */
export function getEntrypointsFromPackageJsonInfo(
packageJsonInfo: PackageJsonInfo,
options: CompilerOptions,
host: ModuleResolutionHost,
host: GetPackageJsonEntrypointsHost,
cache: ModuleResolutionCache | undefined,
resolveJs?: boolean,
): string[] | false {
Expand Down Expand Up @@ -2119,7 +2137,7 @@ export function getEntrypointsFromPackageJsonInfo(
arrayIsEqualTo
);
for (const conditions of conditionSets) {
const loadPackageJsonExportsState = { ...loadPackageJsonMainState, failedLookupLocations: [], conditions };
const loadPackageJsonExportsState = { ...loadPackageJsonMainState, failedLookupLocations: [], conditions, host };
const exportResolutions = loadEntrypointsFromExportMap(
packageJsonInfo,
packageJsonInfo.contents.packageJsonContent.exports,
Expand All @@ -2139,7 +2157,7 @@ export function getEntrypointsFromPackageJsonInfo(
function loadEntrypointsFromExportMap(
scope: PackageJsonInfo,
exports: object,
state: ModuleResolutionState,
state: ModuleResolutionState & { host: GetPackageJsonEntrypointsHost },
Copy link
Member

Choose a reason for hiding this comment

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

I was going to ask if this would be better served as a parameter rather than tacked onto the state, but it does seem like this was already happening given what's in useCaseSensitiveFileNames.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn’t adding a host property, it’s narrowing the type of the existing host property.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was just unsure without trying to find this type (which I should have done!)

extensions: Extensions,
): PathAndExtension[] | undefined {
let entrypoints: PathAndExtension[] | undefined;
Expand All @@ -2160,17 +2178,37 @@ function loadEntrypointsFromExportMap(
return entrypoints;

function loadEntrypointsFromTargetExports(target: unknown): boolean | undefined {
if (typeof target === "string" && startsWith(target, "./") && target.indexOf("*") === -1) {
const partsAfterFirst = getPathComponents(target).slice(2);
if (partsAfterFirst.indexOf("..") >= 0 || partsAfterFirst.indexOf(".") >= 0 || partsAfterFirst.indexOf("node_modules") >= 0) {
return false;
if (typeof target === "string" && startsWith(target, "./")) {
if (target.indexOf("*") >= 0 && state.host.readDirectory) {
if (target.indexOf("*") !== target.lastIndexOf("*")) {
return false;
}

state.host.readDirectory(
scope.packageDirectory,
extensionsToExtensionsArray(extensions),
/*excludes*/ undefined,
[changeAnyExtension(target.replace("*", "**/*"), getDeclarationEmitExtensionForPath(target))]
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
).forEach(entry => {
entrypoints = appendIfUnique(entrypoints, {
path: entry,
ext: getAnyExtensionFromPath(entry),
resolvedUsingTsExtension: undefined
});
});
}
const resolvedTarget = combinePaths(scope.packageDirectory, target);
const finalPath = getNormalizedAbsolutePath(resolvedTarget, state.host.getCurrentDirectory?.());
const result = loadFileNameFromPackageJsonField(extensions, finalPath, /*onlyRecordFailures*/ false, state);
if (result) {
entrypoints = appendIfUnique(entrypoints, result, (a, b) => a.path === b.path);
return true;
else {
const partsAfterFirst = getPathComponents(target).slice(2);
if (partsAfterFirst.indexOf("..") >= 0 || partsAfterFirst.indexOf(".") >= 0 || partsAfterFirst.indexOf("node_modules") >= 0) {
return false;
}
const resolvedTarget = combinePaths(scope.packageDirectory, target);
const finalPath = getNormalizedAbsolutePath(resolvedTarget, state.host.getCurrentDirectory?.());
const result = loadFileNameFromPackageJsonField(extensions, finalPath, /*onlyRecordFailures*/ false, state);
if (result) {
entrypoints = appendIfUnique(entrypoints, result, (a, b) => a.path === b.path);
return true;
}
}
}
else if (Array.isArray(target)) {
Expand Down Expand Up @@ -2675,12 +2713,6 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
return ensureTrailingDirectorySeparator(combinePaths(root, dir));
}

function useCaseSensitiveFileNames() {
return !state.host.useCaseSensitiveFileNames ? true :
typeof state.host.useCaseSensitiveFileNames === "boolean" ? state.host.useCaseSensitiveFileNames :
state.host.useCaseSensitiveFileNames();
}

function tryLoadInputFileForPath(finalPath: string, entry: string, packagePath: string, isImports: boolean) {
// Replace any references to outputs for files in the program with the input files to support package self-names used with outDir
// PROBLEM: We don't know how to calculate the output paths yet, because the "common source directory" we use as the base of the file structure
Expand All @@ -2690,13 +2722,13 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
if (!state.isConfigLookup
&& (state.compilerOptions.declarationDir || state.compilerOptions.outDir)
&& finalPath.indexOf("/node_modules/") === -1
&& (state.compilerOptions.configFile ? containsPath(scope.packageDirectory, toAbsolutePath(state.compilerOptions.configFile.fileName), !useCaseSensitiveFileNames()) : true)
&& (state.compilerOptions.configFile ? containsPath(scope.packageDirectory, toAbsolutePath(state.compilerOptions.configFile.fileName), !useCaseSensitiveFileNames(state)) : true)
) {
// So that all means we'll only try these guesses for files outside `node_modules` in a directory where the `package.json` and `tsconfig.json` are siblings.
// Even with all that, we still don't know if the root of the output file structure will be (relative to the package file)
// `.`, `./src` or any other deeper directory structure. (If project references are used, it's definitely `.` by fiat, so that should be pretty common.)

const getCanonicalFileName = hostGetCanonicalFileName({ useCaseSensitiveFileNames });
const getCanonicalFileName = hostGetCanonicalFileName({ useCaseSensitiveFileNames: () => useCaseSensitiveFileNames(state) });
const commonSourceDirGuesses: string[] = [];
// A `rootDir` compiler option strongly indicates the root location
// A `composite` project is using project references and has it's common src dir set to `.`, so it shouldn't need to check any other locations
Expand Down Expand Up @@ -2751,7 +2783,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
for (const commonSourceDirGuess of commonSourceDirGuesses) {
const candidateDirectories = getOutputDirectoriesForBaseDirectory(commonSourceDirGuess);
for (const candidateDir of candidateDirectories) {
if (containsPath(candidateDir, finalPath, !useCaseSensitiveFileNames())) {
if (containsPath(candidateDir, finalPath, !useCaseSensitiveFileNames(state))) {
// The matched export is looking up something in either the out declaration or js dir, now map the written path back into the source dir and source extension
const pathFragment = finalPath.slice(candidateDir.length + 1); // +1 to also remove directory seperator
const possibleInputBase = combinePaths(commonSourceDirGuess, pathFragment);
Expand All @@ -2761,7 +2793,7 @@ function getLoadModuleFromTargetImportOrExport(extensions: Extensions, state: Mo
const inputExts = getPossibleOriginalInputExtensionForExtension(possibleInputBase);
for (const possibleExt of inputExts) {
if (!extensionIsOk(extensions, possibleExt)) continue;
const possibleInputWithInputExtension = changeAnyExtension(possibleInputBase, possibleExt, ext, !useCaseSensitiveFileNames());
const possibleInputWithInputExtension = changeAnyExtension(possibleInputBase, possibleExt, ext, !useCaseSensitiveFileNames(state));
if (state.host.fileExists(possibleInputWithInputExtension)) {
return toSearchResult(withPackageId(scope, loadFileNameFromPackageJsonField(extensions, possibleInputWithInputExtension, /*onlyRecordFailures*/ false, state)));
}
Expand Down Expand Up @@ -3193,3 +3225,9 @@ function traceIfEnabled(state: ModuleResolutionState, diagnostic: DiagnosticMess
trace(state.host, diagnostic, ...args);
}
}

function useCaseSensitiveFileNames(state: ModuleResolutionState) {
return !state.host.useCaseSensitiveFileNames ? true :
typeof state.host.useCaseSensitiveFileNames === "boolean" ? state.host.useCaseSensitiveFileNames :
state.host.useCaseSensitiveFileNames();
}
24 changes: 13 additions & 11 deletions src/server/project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
getEntrypointsFromPackageJsonInfo,
getNormalizedAbsolutePath,
getOrUpdate,
GetPackageJsonEntrypointsHost,
getStringComparer,
HasInvalidatedLibResolutions,
HasInvalidatedResolutions,
Expand Down Expand Up @@ -2093,7 +2094,7 @@ export abstract class Project implements LanguageServiceHost, ModuleResolutionHo
}

/** @internal */
getModuleResolutionHostForAutoImportProvider(): ModuleResolutionHost {
getHostForAutoImportProvider(): GetPackageJsonEntrypointsHost {
if (this.program) {
return {
fileExists: this.program.fileExists,
Expand All @@ -2104,6 +2105,7 @@ export abstract class Project implements LanguageServiceHost, ModuleResolutionHo
getDirectories: this.projectService.host.getDirectories.bind(this.projectService.host),
trace: this.projectService.host.trace?.bind(this.projectService.host),
useCaseSensitiveFileNames: this.program.useCaseSensitiveFileNames(),
readDirectory: this.projectService.host.readDirectory.bind(this.projectService.host),
};
}
return this.projectService.host;
Expand Down Expand Up @@ -2132,7 +2134,7 @@ export abstract class Project implements LanguageServiceHost, ModuleResolutionHo
if (dependencySelection) {
tracing?.push(tracing.Phase.Session, "getPackageJsonAutoImportProvider");
const start = timestamp();
this.autoImportProviderHost = AutoImportProviderProject.create(dependencySelection, this, this.getModuleResolutionHostForAutoImportProvider(), this.documentRegistry);
this.autoImportProviderHost = AutoImportProviderProject.create(dependencySelection, this, this.getHostForAutoImportProvider(), this.documentRegistry);
if (this.autoImportProviderHost) {
updateProjectIfDirty(this.autoImportProviderHost);
this.sendPerformanceEvent("CreatePackageJsonAutoImportProvider", timestamp() - start);
Expand Down Expand Up @@ -2384,7 +2386,7 @@ export class AutoImportProviderProject extends Project {
private static readonly maxDependencies = 10;

/** @internal */
static getRootFileNames(dependencySelection: PackageJsonAutoImportPreference, hostProject: Project, moduleResolutionHost: ModuleResolutionHost, compilerOptions: CompilerOptions): string[] {
static getRootFileNames(dependencySelection: PackageJsonAutoImportPreference, hostProject: Project, host: GetPackageJsonEntrypointsHost, compilerOptions: CompilerOptions): string[] {
if (!dependencySelection) {
return ts.emptyArray;
}
Expand Down Expand Up @@ -2422,7 +2424,7 @@ export class AutoImportProviderProject extends Project {
name,
hostProject.currentDirectory,
compilerOptions,
moduleResolutionHost,
host,
program.getModuleResolutionCache());
if (packageJson) {
const entrypoints = getRootNamesFromPackageJson(packageJson, program, symlinkCache);
Expand All @@ -2441,7 +2443,7 @@ export class AutoImportProviderProject extends Project {
`@types/${name}`,
directory,
compilerOptions,
moduleResolutionHost,
host,
program.getModuleResolutionCache());
if (typesPackageJson) {
const entrypoints = getRootNamesFromPackageJson(typesPackageJson, program, symlinkCache);
Expand Down Expand Up @@ -2480,11 +2482,11 @@ export class AutoImportProviderProject extends Project {
const entrypoints = getEntrypointsFromPackageJsonInfo(
packageJson,
compilerOptions,
moduleResolutionHost,
host,
program.getModuleResolutionCache(),
resolveJs);
if (entrypoints) {
const real = moduleResolutionHost.realpath?.(packageJson.packageDirectory);
const real = host.realpath?.(packageJson.packageDirectory);
const isSymlink = real && real !== packageJson.packageDirectory;
if (isSymlink) {
symlinkCache.setSymlinkedDirectory(packageJson.packageDirectory, {
Expand Down Expand Up @@ -2514,7 +2516,7 @@ export class AutoImportProviderProject extends Project {
};

/** @internal */
static create(dependencySelection: PackageJsonAutoImportPreference, hostProject: Project, moduleResolutionHost: ModuleResolutionHost, documentRegistry: DocumentRegistry): AutoImportProviderProject | undefined {
static create(dependencySelection: PackageJsonAutoImportPreference, hostProject: Project, host: GetPackageJsonEntrypointsHost, documentRegistry: DocumentRegistry): AutoImportProviderProject | undefined {
if (dependencySelection === PackageJsonAutoImportPreference.Off) {
return undefined;
}
Expand All @@ -2524,7 +2526,7 @@ export class AutoImportProviderProject extends Project {
...this.compilerOptionsOverrides,
};

const rootNames = this.getRootFileNames(dependencySelection, hostProject, moduleResolutionHost, compilerOptions);
const rootNames = this.getRootFileNames(dependencySelection, hostProject, host, compilerOptions);
if (!rootNames.length) {
return undefined;
}
Expand Down Expand Up @@ -2573,7 +2575,7 @@ export class AutoImportProviderProject extends Project {
rootFileNames = AutoImportProviderProject.getRootFileNames(
this.hostProject.includePackageJsonAutoImports(),
this.hostProject,
this.hostProject.getModuleResolutionHostForAutoImportProvider(),
this.hostProject.getHostForAutoImportProvider(),
this.getCompilationSettings());
}

Expand Down Expand Up @@ -2620,7 +2622,7 @@ export class AutoImportProviderProject extends Project {
throw new Error("package.json changes should be notified on an AutoImportProvider's host project");
}

override getModuleResolutionHostForAutoImportProvider(): never {
override getHostForAutoImportProvider(): never {
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been internal to begin with

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s never been clear to me what parts of TS Server are intended to be public API. It seems like this whole class could be internal.

throw new Error("AutoImportProviderProject cannot provide its own host; use `hostProject.getModuleResolutionHostForAutomImportProvider()` instead.");
}

Expand Down
83 changes: 83 additions & 0 deletions tests/cases/fourslash/server/autoImportProvider_wildcardExports1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/// <reference path="../fourslash.ts" />

// @Filename: /node_modules/pkg/package.json
//// {
//// "name": "pkg",
//// "version": "1.0.0",
//// "exports": {
//// "./*": "./a/*.js",
//// "./b/*.js": "./b/*.js",
//// "./c/*": "./c/*"
//// }
//// }

// @Filename: /node_modules/pkg/a/a1.d.ts
//// export const a1: number;

// @Filename: /node_modules/pkg/b/b1.d.ts
//// export const b1: number;

// @Filename: /node_modules/pkg/b/b2.d.mts
//// export const NOT_REACHABLE: number;

// @Filename: /node_modules/pkg/c/c1.d.ts
//// export const c1: number;

// @Filename: /node_modules/pkg/c/subfolder/c2.d.mts
//// export const c2: number;

// @Filename: /package.json
//// {
//// "type": "module",
//// "dependencies": {
//// "pkg": "1.0.0"
//// }
//// }

// @Filename: /tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "nodenext"
//// }
//// }

// @Filename: /main.ts
//// /**/

verify.completions({
marker: "",
includes: [
{
name: "a1",
source: "pkg/a1",
sourceDisplay: "pkg/a1",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
},
{
name: "b1",
source: "pkg/b/b1.js",
sourceDisplay: "pkg/b/b1.js",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
},
{
name: "c1",
source: "pkg/c/c1.js",
sourceDisplay: "pkg/c/c1.js",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
},
{
name: "c2",
source: "pkg/c/subfolder/c2.mjs",
sourceDisplay: "pkg/c/subfolder/c2.mjs",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
}
],
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true
}
});