Skip to content

Commit 2458c8a

Browse files
sheetalkamatsheetalkamat
authored
When the imported module is through node_modules and symlink to folder that isnt node_modules (#37387)
* Add tests that fail because of symlink to non common directory node_modules * When the imported module is through node_modules and symlink to folder that isnt node_modules Most of the monorepo like scenarios are like this so looking at symlink to decide if file can be imported is essential Fixes #28689
1 parent b8baf48 commit 2458c8a

File tree

6 files changed

+230
-51
lines changed

6 files changed

+230
-51
lines changed

‎src/compiler/moduleSpecifiers.ts

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -178,40 +178,70 @@ namespace ts.moduleSpecifiers {
178178
);
179179
}
180180

181-
/**
182-
* Looks for existing imports that use symlinks to this module.
183-
* Symlinks will be returned first so they are preferred over the real path.
184-
*/
185-
function getAllModulePaths(files: readonly SourceFile[], importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): readonly string[] {
181+
export function forEachFileNameOfModule<T>(
182+
files: readonly SourceFile[],
183+
importingFileName: string,
184+
importedFileName: string,
185+
getCanonicalFileName: GetCanonicalFileName,
186+
host: ModuleSpecifierResolutionHost,
187+
redirectTargetsMap: RedirectTargetsMap,
188+
preferSymlinks: boolean,
189+
cb: (fileName: string) => T | undefined
190+
): T | undefined {
186191
const redirects = redirectTargetsMap.get(importedFileName);
187192
const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
188193
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
189194
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
195+
if (!preferSymlinks) {
196+
const result = forEach(targets, cb);
197+
if (result) return result;
198+
}
190199
const links = host.getProbableSymlinks
191200
? host.getProbableSymlinks(files)
192201
: discoverProbableSymlinks(files, getCanonicalFileName, cwd);
193202

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

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

204212
const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
205213
const option = resolvePath(path, relative);
206214
if (!host.fileExists || host.fileExists(option)) {
207-
result.push(option);
215+
const result = cb(option);
216+
if (result) return result;
208217
}
209218
});
210-
result.push(...targets);
211-
if (result.length < 2) return result;
219+
return result ||
220+
(preferSymlinks ? forEach(targets, cb) : undefined);
221+
}
222+
223+
/**
224+
* Looks for existing imports that use symlinks to this module.
225+
* Symlinks will be returned first so they are preferred over the real path.
226+
*/
227+
function getAllModulePaths(files: readonly SourceFile[], importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): readonly string[] {
228+
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
229+
const allFileNames = createMap<string>();
230+
forEachFileNameOfModule(
231+
files,
232+
importingFileName,
233+
importedFileName,
234+
getCanonicalFileName,
235+
host,
236+
redirectTargetsMap,
237+
/*preferSymlinks*/ true,
238+
path => {
239+
// dont return value, so we collect everything
240+
allFileNames.set(path, getCanonicalFileName(path));
241+
}
242+
);
212243

213244
// Sort by paths closest to importing file Name directory
214-
const allFileNames = arrayToMap(result, identity, getCanonicalFileName);
215245
const sortedPaths: string[] = [];
216246
for (
217247
let directory = getDirectoryPath(toPath(importingFileName, cwd, getCanonicalFileName));

‎src/harness/fourslashImpl.ts

Lines changed: 39 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ namespace FourSlash {
2626

2727
files: FourSlashFile[];
2828

29+
symlinks: vfs.FileSet | undefined;
30+
2931
// A mapping from marker names to name/position pairs
3032
markerPositions: ts.Map<Marker>;
3133

@@ -342,6 +344,10 @@ namespace FourSlash {
342344
});
343345
}
344346

347+
if (testData.symlinks) {
348+
this.languageServiceAdapterHost.vfs.apply(testData.symlinks);
349+
}
350+
345351
this.formatCodeSettings = ts.testFormatSettings;
346352

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

37723779
// Split up the input file by line
@@ -3815,32 +3822,38 @@ namespace FourSlash {
38153822
throw new Error("Three-slash line in the middle of four-slash region at line " + i);
38163823
}
38173824
else if (line.substr(0, 2) === "//") {
3818-
// Comment line, check for global/file @options and record them
3819-
const match = optionRegex.exec(line.substr(2));
3820-
if (match) {
3821-
const key = match[1].toLowerCase();
3822-
const value = match[2];
3823-
if (!ts.contains(fileMetadataNames, key)) {
3824-
// Check if the match is already existed in the global options
3825-
if (globalOptions[key] !== undefined) {
3826-
throw new Error(`Global option '${key}' already exists`);
3825+
const possiblySymlinks = Harness.TestCaseParser.parseSymlinkFromTest(line, symlinks);
3826+
if (possiblySymlinks) {
3827+
symlinks = possiblySymlinks;
3828+
}
3829+
else {
3830+
// Comment line, check for global/file @options and record them
3831+
const match = optionRegex.exec(line.substr(2));
3832+
if (match) {
3833+
const key = match[1].toLowerCase();
3834+
const value = match[2];
3835+
if (!ts.contains(fileMetadataNames, key)) {
3836+
// Check if the match is already existed in the global options
3837+
if (globalOptions[key] !== undefined) {
3838+
throw new Error(`Global option '${key}' already exists`);
3839+
}
3840+
globalOptions[key] = value;
38273841
}
3828-
globalOptions[key] = value;
3829-
}
3830-
else {
3831-
switch (key) {
3832-
case MetadataOptionNames.fileName:
3833-
// Found an @FileName directive, if this is not the first then create a new subfile
3834-
nextFile();
3835-
currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
3836-
currentFileOptions[key] = value;
3837-
break;
3838-
case MetadataOptionNames.symlink:
3839-
currentFileSymlinks = ts.append(currentFileSymlinks, value);
3840-
break;
3841-
default:
3842-
// Add other fileMetadata flag
3843-
currentFileOptions[key] = value;
3842+
else {
3843+
switch (key) {
3844+
case MetadataOptionNames.fileName:
3845+
// Found an @FileName directive, if this is not the first then create a new subfile
3846+
nextFile();
3847+
currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
3848+
currentFileOptions[key] = value;
3849+
break;
3850+
case MetadataOptionNames.symlink:
3851+
currentFileSymlinks = ts.append(currentFileSymlinks, value);
3852+
break;
3853+
default:
3854+
// Add other fileMetadata flag
3855+
currentFileOptions[key] = value;
3856+
}
38443857
}
38453858
}
38463859
}
@@ -3870,6 +3883,7 @@ namespace FourSlash {
38703883
markers,
38713884
globalOptions,
38723885
files,
3886+
symlinks,
38733887
ranges
38743888
};
38753889
}

‎src/harness/harnessIO.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,6 +1128,16 @@ namespace Harness {
11281128
const optionRegex = /^[\/]{2}\s*@(\w+)\s*:\s*([^\r\n]*)/gm; // multiple matches on multiple lines
11291129
const linkRegex = /^[\/]{2}\s*@link\s*:\s*([^\r\n]*)\s*->\s*([^\r\n]*)/gm; // multiple matches on multiple lines
11301130

1131+
export function parseSymlinkFromTest(line: string, symlinks: vfs.FileSet | undefined) {
1132+
const linkMetaData = linkRegex.exec(line);
1133+
linkRegex.lastIndex = 0;
1134+
if (!linkMetaData) return undefined;
1135+
1136+
if (!symlinks) symlinks = {};
1137+
symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
1138+
return symlinks;
1139+
}
1140+
11311141
export function extractCompilerSettings(content: string): CompilerSettings {
11321142
const opts: CompilerSettings = {};
11331143

@@ -1163,11 +1173,9 @@ namespace Harness {
11631173

11641174
for (const line of lines) {
11651175
let testMetaData: RegExpExecArray | null;
1166-
const linkMetaData = linkRegex.exec(line);
1167-
linkRegex.lastIndex = 0;
1168-
if (linkMetaData) {
1169-
if (!symlinks) symlinks = {};
1170-
symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
1176+
const possiblySymlinks = parseSymlinkFromTest(line, symlinks);
1177+
if (possiblySymlinks) {
1178+
symlinks = possiblySymlinks;
11711179
}
11721180
else if (testMetaData = optionRegex.exec(line)) {
11731181
// Comment line, check for global/file @options and record them

‎src/services/codefixes/importFixes.ts

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -786,9 +786,9 @@ namespace ts.codefix {
786786
cb: (module: Symbol) => void,
787787
) {
788788
let filteredCount = 0;
789-
const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host);
789+
const moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host);
790+
const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host, moduleSpecifierResolutionHost);
790791
const allSourceFiles = program.getSourceFiles();
791-
const globalTypingsCache = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
792792
forEachExternalModule(program.getTypeChecker(), allSourceFiles, (module, sourceFile) => {
793793
if (sourceFile === undefined) {
794794
if (!packageJson || packageJson.allowsImportingAmbientModule(module, allSourceFiles)) {
@@ -800,7 +800,7 @@ namespace ts.codefix {
800800
}
801801
else if (sourceFile &&
802802
sourceFile !== from &&
803-
isImportablePath(from.fileName, sourceFile.fileName, hostGetCanonicalFileName(host), globalTypingsCache)
803+
isImportableFile(program, from, sourceFile, moduleSpecifierResolutionHost)
804804
) {
805805
if (!packageJson || packageJson.allowsImportingSourceFile(sourceFile, allSourceFiles)) {
806806
cb(module);
@@ -826,6 +826,32 @@ namespace ts.codefix {
826826
}
827827
}
828828

829+
function isImportableFile(
830+
program: Program,
831+
from: SourceFile,
832+
to: SourceFile,
833+
moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost
834+
) {
835+
const getCanonicalFileName = hostGetCanonicalFileName(moduleSpecifierResolutionHost);
836+
const globalTypingsCache = moduleSpecifierResolutionHost.getGlobalTypingsCacheLocation?.();
837+
return !!moduleSpecifiers.forEachFileNameOfModule(
838+
program.getSourceFiles(),
839+
from.fileName,
840+
to.fileName,
841+
hostGetCanonicalFileName(moduleSpecifierResolutionHost),
842+
moduleSpecifierResolutionHost,
843+
program.redirectTargetsMap,
844+
/*preferSymlinks*/ false,
845+
toPath => {
846+
const toFile = program.getSourceFile(toPath);
847+
// Determine to import using toPath only if toPath is what we were looking at
848+
// or there doesnt exist the file in the program by the symlink
849+
return (toFile === to || !toFile) &&
850+
isImportablePath(from.fileName, toPath, getCanonicalFileName, globalTypingsCache);
851+
}
852+
);
853+
}
854+
829855
/**
830856
* Don't include something from a `node_modules` that isn't actually reachable by a global import.
831857
* A relative import to node_modules is usually a bad idea.
@@ -870,12 +896,10 @@ namespace ts.codefix {
870896
return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`;
871897
}
872898

873-
function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost) {
874-
const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
875-
const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;
899+
function createModuleSpecifierResolutionHost(program: Program, host: LanguageServiceHost): ModuleSpecifierResolutionHost {
876900
// Mix in `getProbablySymlinks` from Program when host doesn't have it
877901
// in order for non-Project hosts to have a symlinks cache.
878-
const moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost = {
902+
return {
879903
directoryExists: maybeBind(host, host.directoryExists),
880904
fileExists: maybeBind(host, host.fileExists),
881905
getCurrentDirectory: maybeBind(host, host.getCurrentDirectory),
@@ -884,9 +908,14 @@ namespace ts.codefix {
884908
getProbableSymlinks: maybeBind(host, host.getProbableSymlinks) || program.getProbableSymlinks,
885909
getGlobalTypingsCacheLocation: maybeBind(host, host.getGlobalTypingsCacheLocation),
886910
};
911+
}
912+
913+
function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost, moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host)) {
914+
const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
915+
const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;
887916

888917
let usesNodeCoreModules: boolean | undefined;
889-
return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier };
918+
return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier, moduleSpecifierResolutionHost };
890919

891920
function moduleSpecifierIsCoveredByPackageJson(specifier: string) {
892921
const packageName = getNodeModuleRootSpecifier(specifier);

‎tests/cases/fourslash/importFixesWithSymlinkInSiblingRushPnpm.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/// <reference path="fourslash.ts" />
2+
// @experimentalDecorators: true
3+
4+
// @Filename: /project/libraries/dtos/tsconfig.json
5+
// { }
6+
7+
// @Filename: /project/libraries/dtos/src/book.entity.ts
8+
////@Entity()
9+
////export class BookEntity {
10+
//// id: number
11+
////}
12+
13+
// @Filename: /project/libraries/dtos/src/user.entity.ts
14+
////import { Entity } from "mikro-orm"
15+
////@Entity()
16+
////export class UserEntity {
17+
//// id: number
18+
////}
19+
20+
// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/package.json
21+
////{ "name": "mikro-orm", "version": "3.4.1", "typings": "dist/index.d.ts" }
22+
23+
// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/index.d.ts
24+
////export * from "./decorators";
25+
26+
// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/decorators/index.d.ts
27+
////export * from "./entity";
28+
29+
// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm/dist/decorators/entity.d.ts
30+
////export declare function Entity(): Function;
31+
32+
// @link: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/[email protected]/node_modules/mikro-orm -> /project/libraries/dtos/node_modules/mikro-orm
33+
34+
35+
goTo.file("/project/libraries/dtos/src/book.entity.ts");
36+
verify.importFixAtPosition([
37+
getImportFixContent("mikro-orm"),
38+
getImportFixContent("mikro-orm/dist/decorators"),
39+
getImportFixContent("mikro-orm/dist/decorators/entity"),
40+
]);
41+
42+
function getImportFixContent(from: string) {
43+
return `import { Entity } from "${from}";
44+
45+
@Entity()
46+
export class BookEntity {
47+
id: number
48+
}`;
49+
}

0 commit comments

Comments
 (0)