Skip to content

Do not suggest paths inside node_modules/.pnpm as module specifiers #42095

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 5 commits into from
Jan 12, 2021
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
9 changes: 6 additions & 3 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,9 @@ namespace ts.moduleSpecifiers {
const importedFileNames = [...(referenceRedirect ? [referenceRedirect] : emptyArray), importedFileName, ...redirects];
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
if (!preferSymlinks) {
const result = forEach(targets, p => cb(p, referenceRedirect === p));
// Symlinks inside ignored paths are already filtered out of the symlink cache,
// so we only need to remove them from the realpath filenames.
const result = forEach(targets, p => !containsIgnoredPath(p) && cb(p, referenceRedirect === p));
if (result) return result;
}
const links = host.getSymlinkCache
Expand Down Expand Up @@ -306,8 +308,9 @@ namespace ts.moduleSpecifiers {
}
});
});
return result ||
(preferSymlinks ? forEach(targets, p => cb(p, p === referenceRedirect)) : undefined);
return result || (preferSymlinks
? forEach(targets, p => containsIgnoredPath(p) ? undefined : cb(p, p === referenceRedirect))
: undefined);
}

interface ModulePath {
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3760,7 +3760,7 @@ namespace ts {
}

function handleDirectoryCouldBeSymlink(directory: string) {
if (!host.getResolvedProjectReferences()) return;
if (!host.getResolvedProjectReferences() || containsIgnoredPath(directory)) return;

// Because we already watch node_modules, handle symlinks in there
if (!originalRealpath || !stringContains(directory, nodeModulesPathPart)) return;
Expand Down
13 changes: 12 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6104,7 +6104,14 @@ namespace ts {
getSymlinkedFiles: () => symlinkedFiles,
getSymlinkedDirectories: () => symlinkedDirectories,
setSymlinkedFile: (path, real) => (symlinkedFiles || (symlinkedFiles = new Map())).set(path, real),
setSymlinkedDirectory: (path, directory) => (symlinkedDirectories || (symlinkedDirectories = new Map())).set(path, directory),
setSymlinkedDirectory: (path, directory) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should be handled when it is used for module specifier resolution instead of in general in my opinion since we still want to resolve the cache for answering if file is from referenced project for project construction ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think something inside node_modules/.pnpm/ should never be a project reference redirect, which seems to be the only reason beside module specifier resolution that this is used (fileOrDirectoryExistsUsingSource).

Copy link
Member

Choose a reason for hiding this comment

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

You are right because it would resolve completely to actual path even if equivalent of npm link kind of thing is used .

// Large, interconnected dependency graphs in pnpm will have a huge number of symlinks
// where both the realpath and the symlink path are inside node_modules/.pnpm. Since
// this path is never a candidate for a module specifier, we can ignore it entirely.
if (!containsIgnoredPath(path)) {
(symlinkedDirectories || (symlinkedDirectories = new Map())).set(path, directory);
}
}
};
}

Expand Down Expand Up @@ -7067,4 +7074,8 @@ namespace ts {
return false;
}
}

export function containsIgnoredPath(path: string) {
return some(ignoredPaths, p => stringContains(path, p));
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to look at canonical path? eg. look at isIgnoredPath and isInPath in sys.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

I did look at that, but I noticed that where this is used, we were already doing a containsNodeModulesPathPart check without calling getCanonicalFileName or anything.

}
}
2 changes: 1 addition & 1 deletion src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3022,7 +3022,7 @@ namespace FourSlash {
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
}
if (expectedTextArray.length !== actualTextArray.length) {
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}`);
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}:\n\n${actualTextArray.join("\n\n" + "-".repeat(20) + "\n\n")}`);
}
ts.zipWith(expectedTextArray, actualTextArray, (expected, actual, index) => {
if (expected !== actual) {
Expand Down
24 changes: 24 additions & 0 deletions tests/cases/fourslash/autoImportPnpm.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path="fourslash.ts" />

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

// @Filename: /node_modules/.pnpm/[email protected]/node_modules/mobx/package.json
//// { "types": "dist/mobx.d.ts" }

// @Filename: /node_modules/.pnpm/[email protected]/node_modules/mobx/dist/mobx.d.ts
//// export declare function autorun(): void;

// @Filename: /index.ts
//// autorun/**/

// @Filename: /utils.ts
//// import "mobx";

// @link: /node_modules/.pnpm/[email protected]/node_modules/mobx -> /node_modules/mobx
// @link: /node_modules/.pnpm/[email protected]/node_modules/mobx -> /node_modules/.pnpm/[email protected]/node_modules/mobx

goTo.marker("");
verify.importFixAtPosition([`import { autorun } from "mobx";

autorun`]);