Skip to content

Fix prioritization of paths specifiers over node_modules package specifiers #60238

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 1 commit into from
Oct 23, 2024
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
74 changes: 36 additions & 38 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,46 +508,44 @@ function computeModuleSpecifiers(
}
}

if (!specifier) {
const local = getLocalModuleSpecifier(
modulePath.path,
info,
compilerOptions,
host,
options.overrideImportMode || importingSourceFile.impliedNodeFormat,
preferences,
/*pathsOnly*/ modulePath.isRedirect,
);
if (!local || forAutoImport && isExcludedByRegex(local, preferences.excludeRegexes)) {
continue;
}
if (modulePath.isRedirect) {
redirectPathsSpecifiers = append(redirectPathsSpecifiers, local);
}
else if (pathIsBareSpecifier(local)) {
if (pathContainsNodeModules(local)) {
// We could be in this branch due to inappropriate use of `baseUrl`, not intentional `paths`
// usage. It's impossible to reason about where to prioritize baseUrl-generated module
// specifiers, but if they contain `/node_modules/`, they're going to trigger a portability
// error, so *at least* don't prioritize those.
relativeSpecifiers = append(relativeSpecifiers, local);
}
else {
pathsSpecifiers = append(pathsSpecifiers, local);
}
}
else if (forAutoImport || !importedFileIsInNodeModules || modulePath.isInNodeModules) {
// Why this extra conditional, not just an `else`? If some path to the file contained
// 'node_modules', but we can't create a non-relative specifier (e.g. "@foo/bar/path/to/file"),
// that means we had to go through a *sibling's* node_modules, not one we can access directly.
// If some path to the file was in node_modules but another was not, this likely indicates that
// we have a monorepo structure with symlinks. In this case, the non-node_modules path is
// probably the realpath, e.g. "../bar/path/to/file", but a relative path to another package
// in a monorepo is probably not portable. So, the module specifier we actually go with will be
// the relative path through node_modules, so that the declaration emitter can produce a
// portability error. (See declarationEmitReexportedSymlinkReference3)
const local = getLocalModuleSpecifier(
modulePath.path,
info,
compilerOptions,
host,
options.overrideImportMode || importingSourceFile.impliedNodeFormat,
preferences,
/*pathsOnly*/ modulePath.isRedirect || !!specifier,
);
if (!local || forAutoImport && isExcludedByRegex(local, preferences.excludeRegexes)) {
continue;
}
if (modulePath.isRedirect) {
redirectPathsSpecifiers = append(redirectPathsSpecifiers, local);
}
else if (pathIsBareSpecifier(local)) {
if (pathContainsNodeModules(local)) {
// We could be in this branch due to inappropriate use of `baseUrl`, not intentional `paths`
// usage. It's impossible to reason about where to prioritize baseUrl-generated module
// specifiers, but if they contain `/node_modules/`, they're going to trigger a portability
// error, so *at least* don't prioritize those.
relativeSpecifiers = append(relativeSpecifiers, local);
}
else {
pathsSpecifiers = append(pathsSpecifiers, local);
}
}
else if (forAutoImport || !importedFileIsInNodeModules || modulePath.isInNodeModules) {
// Why this extra conditional, not just an `else`? If some path to the file contained
// 'node_modules', but we can't create a non-relative specifier (e.g. "@foo/bar/path/to/file"),
// that means we had to go through a *sibling's* node_modules, not one we can access directly.
// If some path to the file was in node_modules but another was not, this likely indicates that
// we have a monorepo structure with symlinks. In this case, the non-node_modules path is
// probably the realpath, e.g. "../bar/path/to/file", but a relative path to another package
// in a monorepo is probably not portable. So, the module specifier we actually go with will be
// the relative path through node_modules, so that the declaration emitter can produce a
// portability error. (See declarationEmitReexportedSymlinkReference3)
relativeSpecifiers = append(relativeSpecifiers, local);
}
}

Expand Down
26 changes: 26 additions & 0 deletions tests/cases/fourslash/autoImportPathsNodeModules.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference path="fourslash.ts" />

// @Filename: tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "amd",
//// "moduleResolution": "node",
//// "rootDir": "ts",
//// "baseUrl": ".",
//// "paths": {
//// "*": ["node_modules/@woltlab/wcf/ts/*"]
//// }
//// },
//// "include": [
//// "ts",
//// "node_modules/@woltlab/wcf/ts",
//// ]
//// }

// @Filename: node_modules/@woltlab/wcf/ts/WoltLabSuite/Core/Component/Dialog.ts
//// export class Dialog {}

// @Filename: ts/main.ts
//// Dialog/**/

verify.importFixModuleSpecifiers("", ["WoltLabSuite/Core/Component/Dialog"]);