Skip to content

When the imported module is through node_modules and symlink to folder that isnt node_modules #37387

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
Mar 16, 2020
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
56 changes: 43 additions & 13 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,40 +178,70 @@ namespace ts.moduleSpecifiers {
);
}

/**
* Looks for existing imports that use symlinks to this module.
* Symlinks will be returned first so they are preferred over the real path.
*/
function getAllModulePaths(files: readonly SourceFile[], importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): readonly string[] {
export function forEachFileNameOfModule<T>(
files: readonly SourceFile[],
importingFileName: string,
importedFileName: string,
getCanonicalFileName: GetCanonicalFileName,
host: ModuleSpecifierResolutionHost,
redirectTargetsMap: RedirectTargetsMap,
preferSymlinks: boolean,
cb: (fileName: string) => T | undefined
): T | undefined {
const redirects = redirectTargetsMap.get(importedFileName);
const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
if (!preferSymlinks) {
const result = forEach(targets, cb);
if (result) return result;
}
const links = host.getProbableSymlinks
? host.getProbableSymlinks(files)
: discoverProbableSymlinks(files, getCanonicalFileName, cwd);

const result: string[] = [];
const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
links.forEach((resolved, path) => {
const result = forEachEntry(links, (resolved, path) => {
if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) {
return; // Don't want to a package to globally import from itself
return undefined; // Don't want to a package to globally import from itself
}

const target = find(targets, t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo);
if (target === undefined) return;
if (target === undefined) return undefined;

const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
const option = resolvePath(path, relative);
if (!host.fileExists || host.fileExists(option)) {
result.push(option);
const result = cb(option);
if (result) return result;
}
});
result.push(...targets);
if (result.length < 2) return result;
return result ||
(preferSymlinks ? forEach(targets, cb) : undefined);
}

/**
* Looks for existing imports that use symlinks to this module.
* Symlinks will be returned first so they are preferred over the real path.
*/
function getAllModulePaths(files: readonly SourceFile[], importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): readonly string[] {
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
const allFileNames = createMap<string>();
forEachFileNameOfModule(
files,
importingFileName,
importedFileName,
getCanonicalFileName,
host,
redirectTargetsMap,
/*preferSymlinks*/ true,
path => {
// dont return value, so we collect everything
allFileNames.set(path, getCanonicalFileName(path));
}
);

// Sort by paths closest to importing file Name directory
const allFileNames = arrayToMap(result, identity, getCanonicalFileName);
const sortedPaths: string[] = [];
for (
let directory = getDirectoryPath(toPath(importingFileName, cwd, getCanonicalFileName));
Expand Down
64 changes: 39 additions & 25 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ namespace FourSlash {

files: FourSlashFile[];

symlinks: vfs.FileSet | undefined;

// A mapping from marker names to name/position pairs
markerPositions: ts.Map<Marker>;

Expand Down Expand Up @@ -342,6 +344,10 @@ namespace FourSlash {
});
}

if (testData.symlinks) {
this.languageServiceAdapterHost.vfs.apply(testData.symlinks);
}

this.formatCodeSettings = ts.testFormatSettings;

// Open the first file by default
Expand Down Expand Up @@ -3767,6 +3773,7 @@ namespace FourSlash {
const files: FourSlashFile[] = [];
// Global options
const globalOptions: { [s: string]: string; } = {};
let symlinks: vfs.FileSet | undefined;
// Marker positions

// Split up the input file by line
Expand Down Expand Up @@ -3815,32 +3822,38 @@ namespace FourSlash {
throw new Error("Three-slash line in the middle of four-slash region at line " + i);
}
else if (line.substr(0, 2) === "//") {
// Comment line, check for global/file @options and record them
const match = optionRegex.exec(line.substr(2));
if (match) {
const key = match[1].toLowerCase();
const value = match[2];
if (!ts.contains(fileMetadataNames, key)) {
// Check if the match is already existed in the global options
if (globalOptions[key] !== undefined) {
throw new Error(`Global option '${key}' already exists`);
const possiblySymlinks = Harness.TestCaseParser.parseSymlinkFromTest(line, symlinks);
if (possiblySymlinks) {
symlinks = possiblySymlinks;
}
else {
// Comment line, check for global/file @options and record them
const match = optionRegex.exec(line.substr(2));
if (match) {
const key = match[1].toLowerCase();
const value = match[2];
if (!ts.contains(fileMetadataNames, key)) {
// Check if the match is already existed in the global options
if (globalOptions[key] !== undefined) {
throw new Error(`Global option '${key}' already exists`);
}
globalOptions[key] = value;
}
globalOptions[key] = value;
}
else {
switch (key) {
case MetadataOptionNames.fileName:
// Found an @FileName directive, if this is not the first then create a new subfile
nextFile();
currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
currentFileOptions[key] = value;
break;
case MetadataOptionNames.symlink:
currentFileSymlinks = ts.append(currentFileSymlinks, value);
break;
default:
// Add other fileMetadata flag
currentFileOptions[key] = value;
else {
switch (key) {
case MetadataOptionNames.fileName:
// Found an @FileName directive, if this is not the first then create a new subfile
nextFile();
currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
currentFileOptions[key] = value;
break;
case MetadataOptionNames.symlink:
currentFileSymlinks = ts.append(currentFileSymlinks, value);
break;
default:
// Add other fileMetadata flag
currentFileOptions[key] = value;
}
}
}
}
Expand Down Expand Up @@ -3870,6 +3883,7 @@ namespace FourSlash {
markers,
globalOptions,
files,
symlinks,
ranges
};
}
Expand Down
18 changes: 13 additions & 5 deletions src/harness/harnessIO.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,16 @@ namespace Harness {
const optionRegex = /^[\/]{2}\s*@(\w+)\s*:\s*([^\r\n]*)/gm; // multiple matches on multiple lines
const linkRegex = /^[\/]{2}\s*@link\s*:\s*([^\r\n]*)\s*->\s*([^\r\n]*)/gm; // multiple matches on multiple lines

export function parseSymlinkFromTest(line: string, symlinks: vfs.FileSet | undefined) {
const linkMetaData = linkRegex.exec(line);
linkRegex.lastIndex = 0;
if (!linkMetaData) return undefined;

if (!symlinks) symlinks = {};
symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
return symlinks;
}

export function extractCompilerSettings(content: string): CompilerSettings {
const opts: CompilerSettings = {};

Expand Down Expand Up @@ -1163,11 +1173,9 @@ namespace Harness {

for (const line of lines) {
let testMetaData: RegExpExecArray | null;
const linkMetaData = linkRegex.exec(line);
linkRegex.lastIndex = 0;
if (linkMetaData) {
if (!symlinks) symlinks = {};
symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
const possiblySymlinks = parseSymlinkFromTest(line, symlinks);
if (possiblySymlinks) {
symlinks = possiblySymlinks;
}
else if (testMetaData = optionRegex.exec(line)) {
// Comment line, check for global/file @options and record them
Expand Down
45 changes: 37 additions & 8 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -786,9 +786,9 @@ namespace ts.codefix {
cb: (module: Symbol) => void,
) {
let filteredCount = 0;
const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host);
const moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host);
const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host, moduleSpecifierResolutionHost);
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 @@ -800,7 +800,7 @@ namespace ts.codefix {
}
else if (sourceFile &&
sourceFile !== from &&
isImportablePath(from.fileName, sourceFile.fileName, hostGetCanonicalFileName(host), globalTypingsCache)
isImportableFile(program, from, sourceFile, moduleSpecifierResolutionHost)
) {
if (!packageJson || packageJson.allowsImportingSourceFile(sourceFile, allSourceFiles)) {
cb(module);
Expand All @@ -826,6 +826,32 @@ namespace ts.codefix {
}
}

function isImportableFile(
program: Program,
from: SourceFile,
to: SourceFile,
moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost
) {
const getCanonicalFileName = hostGetCanonicalFileName(moduleSpecifierResolutionHost);
const globalTypingsCache = moduleSpecifierResolutionHost.getGlobalTypingsCacheLocation?.();
return !!moduleSpecifiers.forEachFileNameOfModule(
program.getSourceFiles(),
from.fileName,
to.fileName,
hostGetCanonicalFileName(moduleSpecifierResolutionHost),
moduleSpecifierResolutionHost,
program.redirectTargetsMap,
/*preferSymlinks*/ false,
toPath => {
const toFile = program.getSourceFile(toPath);
// Determine to import using toPath only if toPath is what we were looking at
// or there doesnt exist the file in the program by the symlink
return (toFile === to || !toFile) &&
isImportablePath(from.fileName, toPath, getCanonicalFileName, globalTypingsCache);
}
);
}

/**
* 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.
Expand Down Expand Up @@ -870,12 +896,10 @@ namespace ts.codefix {
return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`;
}

function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost) {
const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;
function createModuleSpecifierResolutionHost(program: Program, host: LanguageServiceHost): ModuleSpecifierResolutionHost {
// Mix in `getProbablySymlinks` from Program when host doesn't have it
// in order for non-Project hosts to have a symlinks cache.
const moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost = {
return {
directoryExists: maybeBind(host, host.directoryExists),
fileExists: maybeBind(host, host.fileExists),
getCurrentDirectory: maybeBind(host, host.getCurrentDirectory),
Expand All @@ -884,9 +908,14 @@ namespace ts.codefix {
getProbableSymlinks: maybeBind(host, host.getProbableSymlinks) || program.getProbableSymlinks,
getGlobalTypingsCacheLocation: maybeBind(host, host.getGlobalTypingsCacheLocation),
};
}

function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost, moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host)) {
const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;

let usesNodeCoreModules: boolean | undefined;
return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier };
return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier, moduleSpecifierResolutionHost };

function moduleSpecifierIsCoveredByPackageJson(specifier: string) {
const packageName = getNodeModuleRootSpecifier(specifier);
Expand Down
49 changes: 49 additions & 0 deletions tests/cases/fourslash/importFixesWithSymlinkInSiblingRushPnpm.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/// <reference path="fourslash.ts" />
// @experimentalDecorators: true

// @Filename: /project/libraries/dtos/tsconfig.json
// { }

// @Filename: /project/libraries/dtos/src/book.entity.ts
////@Entity()
////export class BookEntity {
//// id: number
////}

// @Filename: /project/libraries/dtos/src/user.entity.ts
////import { Entity } from "mikro-orm"
////@Entity()
////export class UserEntity {
//// id: number
////}

// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/package.json
////{ "name": "mikro-orm", "version": "3.4.1", "typings": "dist/index.d.ts" }

// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/index.d.ts
////export * from "./decorators";

// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/decorators/index.d.ts
////export * from "./entity";

// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/decorators/entity.d.ts
////export declare function Entity(): Function;

// @link: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm -> /project/libraries/dtos/node_modules/mikro-orm


goTo.file("/project/libraries/dtos/src/book.entity.ts");
verify.importFixAtPosition([
getImportFixContent("mikro-orm"),
getImportFixContent("mikro-orm/dist/decorators"),
getImportFixContent("mikro-orm/dist/decorators/entity"),
]);

function getImportFixContent(from: string) {
return `import { Entity } from "${from}";

@Entity()
export class BookEntity {
id: number
}`;
}
Loading