Skip to content

Fix auto-imports from auto type acquisition definitions #33766

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.

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

Merged
merged 2 commits into from
Oct 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,13 @@ namespace ts.moduleSpecifiers {

// If the module could be imported by a directory name, use that directory's name
const moduleSpecifier = packageNameOnly ? moduleFileName : getDirectoryOrExtensionlessFileName(moduleFileName);
const globalTypingsCacheLocation = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
// Get a path that's relative to node_modules or the importing file's path
// if node_modules folder is in this folder or any of its parent folders, no need to keep it.
if (!startsWith(sourceDirectory, getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex)))) return undefined;
const pathToTopLevelNodeModules = getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex));
if (!(startsWith(sourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {
return undefined;
}

// If the module was found in @types, get the actual Node package name
const nodeModulesDirectoryName = moduleSpecifier.substring(parts.topLevelPackageNameIndex + 1);
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6149,6 +6149,8 @@ namespace ts {
readFile?(path: string): string | undefined;
/* @internal */
getProbableSymlinks?(files: readonly SourceFile[]): ReadonlyMap<string>;
/* @internal */
getGlobalTypingsCacheLocation?(): string | undefined;
}

// Note: this used to be deprecated in our public API, but is still used internally
Expand Down
4 changes: 4 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ namespace Harness.LanguageService {
return !!this.typesRegistry && this.typesRegistry.has(name);
}

getGlobalTypingsCacheLocation() {
return "/Library/Caches/typescript";
Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone have a less kludgy idea for how to test this? I looked into the server fourslash tests, but it would ultimately require the same kind of hard-coded hack, just buried deeper in the infrastructure.

}

installPackage = ts.notImplemented;

getCompilationSettings() { return this.settings; }
Expand Down
5 changes: 5 additions & 0 deletions src/server/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ namespace ts.server {
return this.typingsCache.installPackage({ ...options, projectName: this.projectName, projectRootPath: this.toPath(this.currentDirectory) });
}

/*@internal*/
getGlobalTypingsCacheLocation() {
return this.getGlobalCache();
}

private get typingsCache(): TypingsCache {
return this.projectService.typingsCache;
}
Expand Down
14 changes: 11 additions & 3 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ namespace ts.codefix {
let filteredCount = 0;
const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host);
const allSourceFiles = program.getSourceFiles();
const globalTypingsCache = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
forEachExternalModule(program.getTypeChecker(), allSourceFiles, (module, sourceFile) => {
if (sourceFile === undefined) {
if (!packageJson || packageJson.allowsImportingAmbientModule(module, allSourceFiles)) {
Expand All @@ -640,7 +641,10 @@ namespace ts.codefix {
filteredCount++;
}
}
else if (sourceFile && sourceFile !== from && isImportablePath(from.fileName, sourceFile.fileName)) {
else if (sourceFile &&
sourceFile !== from &&
isImportablePath(from.fileName, sourceFile.fileName, hostGetCanonicalFileName(host), globalTypingsCache)
) {
if (!packageJson || packageJson.allowsImportingSourceFile(sourceFile, allSourceFiles)) {
cb(module);
}
Expand Down Expand Up @@ -669,10 +673,13 @@ namespace ts.codefix {
* Don't include something from a `node_modules` that isn't actually reachable by a global import.
* A relative import to node_modules is usually a bad idea.
*/
function isImportablePath(fromPath: string, toPath: string): boolean {
function isImportablePath(fromPath: string, toPath: string, getCanonicalFileName: GetCanonicalFileName, globalCachePath?: string): boolean {
// If it's in a `node_modules` but is not reachable from here via a global import, don't bother.
const toNodeModules = forEachAncestorDirectory(toPath, ancestor => getBaseFileName(ancestor) === "node_modules" ? ancestor : undefined);
return toNodeModules === undefined || startsWith(fromPath, getDirectoryPath(toNodeModules));
const toNodeModulesParent = toNodeModules && getDirectoryPath(getCanonicalFileName(toNodeModules));
return toNodeModulesParent === undefined
|| startsWith(getCanonicalFileName(fromPath), toNodeModulesParent)
|| (!!globalCachePath && startsWith(getCanonicalFileName(globalCachePath), toNodeModulesParent));
}

export function moduleSymbolToValidIdentifier(moduleSymbol: Symbol, target: ScriptTarget): string {
Expand Down Expand Up @@ -718,6 +725,7 @@ namespace ts.codefix {
readFile: maybeBind(host, host.readFile),
useCaseSensitiveFileNames: maybeBind(host, host.useCaseSensitiveFileNames),
getProbableSymlinks: maybeBind(host, host.getProbableSymlinks) || program.getProbableSymlinks,
getGlobalTypingsCacheLocation: maybeBind(host, host.getGlobalTypingsCacheLocation),
};

let usesNodeCoreModules: boolean | undefined;
Expand Down
2 changes: 2 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ namespace ts {
resolveTypeReferenceDirectives?(typeDirectiveNames: string[], containingFile: string, redirectedReference: ResolvedProjectReference | undefined, options: CompilerOptions): (ResolvedTypeReferenceDirective | undefined)[];
/* @internal */ hasInvalidatedResolution?: HasInvalidatedResolution;
/* @internal */ hasChangedAutomaticTypeDirectiveNames?: boolean;
/* @internal */
getGlobalTypingsCacheLocation?(): string | undefined;

/*
* Required for full import and type reference completions.
Expand Down
18 changes: 18 additions & 0 deletions tests/cases/fourslash/importFixesGlobalTypingsCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path="fourslash.ts" />

// @Filename: /project/tsconfig.json
//// { "compilerOptions": { "allowJs": true, "checkJs": true } }

// @Filename: /Library/Caches/typescript/node_modules/@types/react-router-dom/package.json
//// { "name": "react-router-dom" }

// @Filename: /Library/Caches/typescript/node_modules/@types/react-router-dom/index.d.ts
////export class BrowserRouter {}

// @Filename: /project/index.js
////BrowserRouter/**/

goTo.file("/project/index.js");
verify.importFixAtPosition([`import { BrowserRouter } from "react-router-dom";

BrowserRouter`]);