Skip to content

Commit 33046e3

Browse files
authored
Do not suggest paths inside node_modules/.pnpm as module specifiers (#42095)
* Create symlink cache when a pnpm module is found * Keep pnpm-internal symlinks out of the symlink cache * Filter out pnpm path from realpath module specifier too * Use ignoredPaths instead of pnpm-specific path
1 parent 2f58637 commit 33046e3

File tree

5 files changed

+44
-6
lines changed

5 files changed

+44
-6
lines changed

src/compiler/moduleSpecifiers.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,9 @@ namespace ts.moduleSpecifiers {
290290
const importedFileNames = [...(referenceRedirect ? [referenceRedirect] : emptyArray), importedFileName, ...redirects];
291291
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
292292
if (!preferSymlinks) {
293-
const result = forEach(targets, p => cb(p, referenceRedirect === p));
293+
// Symlinks inside ignored paths are already filtered out of the symlink cache,
294+
// so we only need to remove them from the realpath filenames.
295+
const result = forEach(targets, p => !containsIgnoredPath(p) && cb(p, referenceRedirect === p));
294296
if (result) return result;
295297
}
296298
const links = host.getSymlinkCache
@@ -318,8 +320,9 @@ namespace ts.moduleSpecifiers {
318320
}
319321
});
320322
});
321-
return result ||
322-
(preferSymlinks ? forEach(targets, p => cb(p, p === referenceRedirect)) : undefined);
323+
return result || (preferSymlinks
324+
? forEach(targets, p => containsIgnoredPath(p) ? undefined : cb(p, p === referenceRedirect))
325+
: undefined);
323326
}
324327

325328
interface ModulePath {

src/compiler/program.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3756,7 +3756,7 @@ namespace ts {
37563756
}
37573757

37583758
function handleDirectoryCouldBeSymlink(directory: string) {
3759-
if (!host.getResolvedProjectReferences()) return;
3759+
if (!host.getResolvedProjectReferences() || containsIgnoredPath(directory)) return;
37603760

37613761
// Because we already watch node_modules, handle symlinks in there
37623762
if (!originalRealpath || !stringContains(directory, nodeModulesPathPart)) return;

src/compiler/utilities.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6092,7 +6092,14 @@ namespace ts {
60926092
getSymlinkedFiles: () => symlinkedFiles,
60936093
getSymlinkedDirectories: () => symlinkedDirectories,
60946094
setSymlinkedFile: (path, real) => (symlinkedFiles || (symlinkedFiles = new Map())).set(path, real),
6095-
setSymlinkedDirectory: (path, directory) => (symlinkedDirectories || (symlinkedDirectories = new Map())).set(path, directory),
6095+
setSymlinkedDirectory: (path, directory) => {
6096+
// Large, interconnected dependency graphs in pnpm will have a huge number of symlinks
6097+
// where both the realpath and the symlink path are inside node_modules/.pnpm. Since
6098+
// this path is never a candidate for a module specifier, we can ignore it entirely.
6099+
if (!containsIgnoredPath(path)) {
6100+
(symlinkedDirectories || (symlinkedDirectories = new Map())).set(path, directory);
6101+
}
6102+
}
60966103
};
60976104
}
60986105

@@ -7066,4 +7073,8 @@ namespace ts {
70667073
return false;
70677074
}
70687075
}
7076+
7077+
export function containsIgnoredPath(path: string) {
7078+
return some(ignoredPaths, p => stringContains(path, p));
7079+
}
70697080
}

src/harness/fourslashImpl.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3022,7 +3022,7 @@ namespace FourSlash {
30223022
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
30233023
}
30243024
if (expectedTextArray.length !== actualTextArray.length) {
3025-
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}`);
3025+
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}:\n\n${actualTextArray.join("\n\n" + "-".repeat(20) + "\n\n")}`);
30263026
}
30273027
ts.zipWith(expectedTextArray, actualTextArray, (expected, actual, index) => {
30283028
if (expected !== actual) {
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Filename: /tsconfig.json
4+
//// { "compilerOptions": { "module": "commonjs" } }
5+
6+
// @Filename: /node_modules/.pnpm/[email protected]/node_modules/mobx/package.json
7+
//// { "types": "dist/mobx.d.ts" }
8+
9+
// @Filename: /node_modules/.pnpm/[email protected]/node_modules/mobx/dist/mobx.d.ts
10+
//// export declare function autorun(): void;
11+
12+
// @Filename: /index.ts
13+
//// autorun/**/
14+
15+
// @Filename: /utils.ts
16+
//// import "mobx";
17+
18+
// @link: /node_modules/.pnpm/[email protected]/node_modules/mobx -> /node_modules/mobx
19+
// @link: /node_modules/.pnpm/[email protected]/node_modules/mobx -> /node_modules/.pnpm/[email protected]/node_modules/mobx
20+
21+
goTo.marker("");
22+
verify.importFixAtPosition([`import { autorun } from "mobx";
23+
24+
autorun`]);

0 commit comments

Comments
 (0)