Skip to content
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

Jump to bottom

Generate shortest rootDirs module specifier instead of first possible #51244

Merged
merged 2 commits into from Oct 21, 2022
Merged

Generate shortest rootDirs module specifier instead of first possible #51244

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
06e4597
Generate shortest rootDirs module specifier instead of first possible
andrewbranch Oct 20, 2022
316089d
Simplify `min`
andrewbranch Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/compiler/core.ts
View file
Open in desktop
Expand Up @@ -1937,8 +1937,10 @@ namespace ts {
return compareValues(a?.start, b?.start) || compareValues(a?.length, b?.length);
}

export function min<T>(a: T, b: T, compare: Comparer<T>): T {
return compare(a, b) === Comparison.LessThan ? a : b;
export function min<T>(items: readonly [T, ...T[]], compare: Comparer<T>): T;
export function min<T>(items: readonly T[], compare: Comparer<T>): T | undefined;
export function min<T>(items: readonly T[], compare: Comparer<T>): T | undefined {
return reduceLeft(items, (x, y) => compare(x, y) === Comparison.LessThan ? x : y);
Comment on lines +1940 to +1943
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it have been better to switch the call-site to

reduceLeft(items, min)

leaving the old definition alone?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I guess you would have needed to pass in a comparer.

reduceLeft(items, (x, y) => min(x, y, compareMatches));

Copy link
Member

Choose a reason for hiding this comment

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

Okay, never mind, there are more call sites than just two now. I just have no idea how often betterMatch gets called, so I feel like it might just be better to have two helper functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it’s silly for min to take only two items to compare. It had one call site before; now it has two. At first I preserved the two-argument form and made an overload for the array form, but the function implementation with the combined overload signature was disgusting. I considered inlining the reduce call into the call site, but min is so much more readable. If anything, the old implementation could be inlined into betterMatch. Fewer, better utils seems better to me.

}

/**
Expand Down
23 changes: 15 additions & 8 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Expand Up @@ -726,16 +726,23 @@ namespace ts.moduleSpecifiers {
}

function tryGetModuleNameFromRootDirs(rootDirs: readonly string[], moduleFileName: string, sourceDirectory: string, getCanonicalFileName: (file: string) => string, ending: Ending, compilerOptions: CompilerOptions): string | undefined {
const normalizedTargetPath = getPathRelativeToRootDirs(moduleFileName, rootDirs, getCanonicalFileName);
if (normalizedTargetPath === undefined) {
const normalizedTargetPaths = getPathsRelativeToRootDirs(moduleFileName, rootDirs, getCanonicalFileName);
if (normalizedTargetPaths === undefined) {
return undefined;
}

const normalizedSourcePaths = getPathsRelativeToRootDirs(sourceDirectory, rootDirs, getCanonicalFileName);
const relativePaths = flatMap(normalizedSourcePaths, sourcePath => {
return map(normalizedTargetPaths, targetPath => ensurePathIsNonModuleName(getRelativePathFromDirectory(sourcePath, targetPath, getCanonicalFileName)));
});
const shortest = min(relativePaths, compareNumberOfDirectorySeparators);
if (!shortest) {
return undefined;
}

const normalizedSourcePath = getPathRelativeToRootDirs(sourceDirectory, rootDirs, getCanonicalFileName);
const relativePath = normalizedSourcePath !== undefined ? ensurePathIsNonModuleName(getRelativePathFromDirectory(normalizedSourcePath, normalizedTargetPath, getCanonicalFileName)) : normalizedTargetPath;
return getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.NodeJs
? removeExtensionAndIndexPostFix(relativePath, ending, compilerOptions)
: removeFileExtension(relativePath);
? removeExtensionAndIndexPostFix(shortest, ending, compilerOptions)
: removeFileExtension(shortest);
}

function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, importingSourceFile: SourceFile , host: ModuleSpecifierResolutionHost, options: CompilerOptions, userPreferences: UserPreferences, packageNameOnly?: boolean, overrideMode?: ModuleKind.ESNext | ModuleKind.CommonJS): string | undefined {
Expand Down Expand Up @@ -882,8 +889,8 @@ namespace ts.moduleSpecifiers {
}
}

function getPathRelativeToRootDirs(path: string, rootDirs: readonly string[], getCanonicalFileName: GetCanonicalFileName): string | undefined {
return firstDefined(rootDirs, rootDir => {
function getPathsRelativeToRootDirs(path: string, rootDirs: readonly string[], getCanonicalFileName: GetCanonicalFileName): string[] | undefined {
return mapDefined(rootDirs, rootDir => {
const relativePath = getRelativePathIfInDirectory(path, rootDir, getCanonicalFileName);
return relativePath !== undefined && isPathRelativeToParent(relativePath) ? undefined : relativePath;
});
Expand Down
2 changes: 1 addition & 1 deletion src/services/patternMatcher.ts
View file
Open in desktop
Expand Up @@ -259,7 +259,7 @@ namespace ts {
}

function betterMatch(a: PatternMatch | undefined, b: PatternMatch | undefined): PatternMatch | undefined {
return min(a, b, compareMatches);
return min([a, b], compareMatches);
}
function compareMatches(a: PatternMatch | undefined, b: PatternMatch | undefined): Comparison {
return a === undefined ? Comparison.GreaterThan : b === undefined ? Comparison.LessThan
Expand Down
17 changes: 17 additions & 0 deletions tests/cases/fourslash/autoImportRootDirs.ts
View file
Open in desktop
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />

// @Filename: /tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "commonjs",
//// "rootDirs": [".", "./some/other/root"]
//// }
//// }

// @Filename: /some/other/root/types.ts
//// export type Something = {};

// @Filename: /index.ts
//// const s: Something/**/

verify.importFixModuleSpecifiers("", ["./types"]);