Skip to content

Fix auto-import when paths points to project reference redirect #51492

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.

Sign up for GitHub

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 2 commits into from
Nov 15, 2022
Merged

Fix auto-import when paths points to project reference redirect #51492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d25792
Fix auto-import when `paths` points to project reference redirect
andrewbranch Nov 11, 2022
4f5d243
Put paths specifiers to redirects in lower priority bucket
andrewbranch Nov 11, 2022
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
58 changes: 41 additions & 17 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ function computeModuleSpecifiers(
// 4. Relative paths
let nodeModulesSpecifiers: string[] | undefined;
let pathsSpecifiers: string[] | undefined;
let redirectPathsSpecifiers: string[] | undefined;
let relativeSpecifiers: string[] | undefined;
for (const modulePath of modulePaths) {
const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode);
Expand All @@ -285,9 +286,23 @@ function computeModuleSpecifiers(
return nodeModulesSpecifiers!;
}

if (!specifier && !modulePath.isRedirect) {
const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences);
if (pathIsBareSpecifier(local)) {
if (!specifier) {
const local = getLocalModuleSpecifier(
modulePath.path,
info,
compilerOptions,
host,
options.overrideImportMode || importingSourceFile.impliedNodeFormat,
preferences,
/*pathsOnly*/ modulePath.isRedirect,
);
if (!local) {
continue;
}
if (modulePath.isRedirect) {
redirectPathsSpecifiers = append(redirectPathsSpecifiers, local);
}
else if (pathIsBareSpecifier(local)) {
pathsSpecifiers = append(pathsSpecifiers, local);
}
else if (!importedFileIsInNodeModules || modulePath.isInNodeModules) {
Expand All @@ -306,8 +321,9 @@ function computeModuleSpecifiers(
}

return pathsSpecifiers?.length ? pathsSpecifiers :
redirectPathsSpecifiers?.length ? redirectPathsSpecifiers :
nodeModulesSpecifiers?.length ? nodeModulesSpecifiers :
Debug.checkDefined(relativeSpecifiers);
Debug.checkDefined(relativeSpecifiers);
}

interface Info {
Expand All @@ -322,32 +338,42 @@ function getInfo(importingSourceFileName: Path, host: ModuleSpecifierResolutionH
return { getCanonicalFileName, importingSourceFileName, sourceDirectory };
}

function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences): string {
function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences): string;
function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences, pathsOnly?: boolean): string | undefined;
function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences, pathsOnly?: boolean): string | undefined {
const { baseUrl, paths, rootDirs } = compilerOptions;
if (pathsOnly && !paths) {
return undefined;
}

const { sourceDirectory, getCanonicalFileName } = info;
const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName, ending, compilerOptions) ||
removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), ending, compilerOptions);
if (!baseUrl && !paths || relativePreference === RelativePreference.Relative) {
return relativePath;
return pathsOnly ? undefined : relativePath;
}

const baseDirectory = getNormalizedAbsolutePath(getPathsBasePath(compilerOptions, host) || baseUrl!, host.getCurrentDirectory());
const relativeToBaseUrl = getRelativePathIfInDirectory(moduleFileName, baseDirectory, getCanonicalFileName);
if (!relativeToBaseUrl) {
return relativePath;
return pathsOnly ? undefined : relativePath;
}

const fromPaths = paths && tryGetModuleNameFromPaths(relativeToBaseUrl, paths, getAllowedEndings(ending, compilerOptions, importMode), host, compilerOptions);
const nonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths;
if (!nonRelative) {
if (pathsOnly) {
return fromPaths;
}

const maybeNonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths;
if (!maybeNonRelative) {
return relativePath;
}

if (relativePreference === RelativePreference.NonRelative) {
return nonRelative;
if (relativePreference === RelativePreference.NonRelative && !pathIsRelative(maybeNonRelative)) {
return maybeNonRelative;
}

if (relativePreference === RelativePreference.ExternalNonRelative) {
if (relativePreference === RelativePreference.ExternalNonRelative && !pathIsRelative(maybeNonRelative)) {
const projectDirectory = compilerOptions.configFilePath ?
toPath(getDirectoryPath(compilerOptions.configFilePath), host.getCurrentDirectory(), info.getCanonicalFileName) :
info.getCanonicalFileName(host.getCurrentDirectory());
Expand All @@ -363,7 +389,7 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt
// lib/ | (path crosses tsconfig.json)
// imported.ts <---
//
return nonRelative;
return maybeNonRelative;
}

const nearestTargetPackageJson = getNearestAncestorDirectoryWithPackageJson(host, getDirectoryPath(modulePath));
Expand All @@ -378,16 +404,14 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt
// package.json |
// component.ts <---
//
return nonRelative;
return maybeNonRelative;
}

return relativePath;
}

if (relativePreference !== RelativePreference.Shortest) Debug.assertNever(relativePreference);

// Prefer a relative import over a baseUrl import if it has fewer components.
return isPathRelativeToParent(nonRelative) || countPathComponents(relativePath) < countPathComponents(nonRelative) ? relativePath : nonRelative;
return isPathRelativeToParent(maybeNonRelative) || countPathComponents(relativePath) < countPathComponents(maybeNonRelative) ? relativePath : maybeNonRelative;
}

/** @internal */
Expand Down
4 changes: 4 additions & 0 deletions src/harness/fourslashImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3041,6 +3041,10 @@ export class TestState {
* @param fileName Path to file where error should be retrieved from.
*/
private getCodeFixes(fileName: string, errorCode?: number, preferences: ts.UserPreferences = ts.emptyOptions, position?: number): readonly ts.CodeFixAction[] {
if (this.testType === FourSlashTestType.Server) {
this.configure(preferences);
}

const diagnosticsForCodeFix = this.getDiagnostics(fileName, /*includeSuggestions*/ true).map(diagnostic => ({
start: diagnostic.start,
length: diagnostic.length,
Expand Down
41 changes: 41 additions & 0 deletions tests/cases/fourslash/server/autoImportCrossProject_baseUrl_toDist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/// <reference path="../fourslash.ts" />

// @Filename: /common/tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "commonjs",
//// "outDir": "dist",
//// "composite": true
//// },
//// "include": ["src"]
//// }

// @Filename: /common/src/MyModule.ts
//// export function square(n: number) {
//// return n * 2;
//// }

// @Filename: /web/tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "esnext",
//// "moduleResolution": "node",
//// "noEmit": true,
//// "baseUrl": "."
//// },
//// "include": ["src"],
//// "references": [{ "path": "../common" }]
//// }

// @Filename: /web/src/MyApp.ts
//// import { square } from "../../common/dist/src/MyModule";

// @Filename: /web/src/Helper.ts
//// export function saveMe() {
//// square/**/(2);
//// }

goTo.file("/web/src/Helper.ts");
verify.importFixModuleSpecifiers("", ["../../common/src/MyModule"], {
importModuleSpecifierPreference: "non-relative"
});
43 changes: 43 additions & 0 deletions tests/cases/fourslash/server/autoImportCrossProject_paths_toDist2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/// <reference path="../fourslash.ts" />

// @Filename: /common/tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "commonjs",
//// "outDir": "dist",
//// "composite": true
//// },
//// "include": ["src"]
//// }

// @Filename: /common/src/MyModule.ts
//// export function square(n: number) {
//// return n * 2;
//// }

// @Filename: /web/tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "esnext",
//// "moduleResolution": "node",
//// "noEmit": true,
//// "paths": {
//// "@common/*": ["../common/dist/src/*"]
//// }
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Offtopic question: I was actually exploring using exactly this structure in one of the projects. However, I only want this because Remix doesn't allow me to alias paths - but it can infer aliases from the compilerOptions.paths.

I wonder what are the pros/cons of this structure. I guess that in this test case here it might be required to use non-relative paths as /common/ isn't actually symlinked as a monorepo package. However, when referenced projects are symlinked in node_modules, I expect this to be unnecessary and less performant. In the IDE this might be roughly the same thanks to "behind-the-scenes in-memory .d.ts generation process" but when running tsc -b this might not be able to actually leverage the generated .d.ts files as the paths map to /src/, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, in this example paths map to dist (the original repro didn’t set a rootDir so the output is in dist/src and I kept that quirk in the test). But IIRC whether you map to the inputs or outputs of a referenced project, TypeScript understands what you’re doing and tsc will find the .d.ts outputs and the language service will find the .ts inputs.

//// },
//// "include": ["src"],
//// "references": [{ "path": "../common" }]
//// }

// @Filename: /web/src/MyApp.ts
//// import { square } from "@common/MyModule";

// @Filename: /web/src/Helper.ts
//// export function saveMe() {
//// square/**/(2);
//// }

goTo.file("/web/src/Helper.ts");
verify.importFixModuleSpecifiers("", ["@common/MyModule"], {
importModuleSpecifierPreference: "non-relative"
});