Skip to content

Commit

Permalink
Fix prioritization of paths specifiers over node_modules package sp…
Browse files Browse the repository at this point in the history
…ecifiers (#60238)
  • Loading branch information
andrewbranch authored Oct 23, 2024
1 parent db8eacd commit 2ac4cb7
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 38 deletions.
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"]);

0 comments on commit 2ac4cb7

Please sign in to comment.